Skip to content

Conversation

@lindapaiste
Copy link
Collaborator

Progress on #1458 and #824

Should be merged after #2049 as I moved the Escape key handling into the new Modal component.

Changes:

  • Create new utility hook useKeyDownHandlers for adding one or more keydown handlers to the document, and component DocumentKeyDown which wraps the hook for use in class components.
  • Apply hook/component to Nav, Overlay, and Modal
  • Move the keydown handling from IDEView into new hook/component useIDEKeyHandlers/IDEKeyHandlers, which accesses redux props directly and allows a few props to be removed from IDEView.
  • Use the same keydown handling in MobileIDEView instead of repeating it.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@raclim
Copy link
Collaborator

raclim commented Aug 10, 2023

This looks great for me so far! I think the only issue I've noticed is that the sketch will still run despite using the Keyboard shortcut ⌘ + ⇧ + Enter to stop it.

@dewanshDT dewanshDT mentioned this pull request Aug 17, 2023
3 tasks
Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I found the source of my issue with stopping the sketch, which was in lines 34-38 in the useKeyDownHandlers.js file. Within those lines, I think the order in which the handlers were being called might not be consistent across different browsers/devices, which is why my sketch would restart again. I updated the check to use an else if statement, and also removed a few linting warnings and errors.

I'm sorry again that this took a while to review and thank you for making these really awesome changes!

@raclim raclim merged commit 2eb8690 into processing:develop Aug 23, 2023
@lindapaiste
Copy link
Collaborator Author

@raclim I need to put in a quick bug fix because apparently e.key is not always defined 🤦 . It might just be Chrome but the autocomplete form filling sends a keydown event with no e.key which causes an error.

image
image

@raclim
Copy link
Collaborator

raclim commented Aug 28, 2023

@lindapaiste, sounds good, thanks for catching this and giving a heads up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants