Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with Escape key while editing link #1215

Merged
merged 2 commits into from
Jun 16, 2017
Merged

Fix issues with Escape key while editing link #1215

merged 2 commits into from
Jun 16, 2017

Conversation

mkaz
Copy link
Member

@mkaz mkaz commented Jun 16, 2017

  • Switch to using event.key instead of event.keyCode which is deprecated
  • Add event.stopPropagation() instead of default
  • Add remove event listener on unmount
  • Use keydown event not keypress - keypress doesnt work with Escape key

* Switch to using event.key instead of event.keyCode which is deprecated
* Add event.stopPropagation() instead of default
* Add remove event listener on unmount
* Use keydown event not keypress - keypress doesnt work with Escape key
@aduth
Copy link
Member

aduth commented Jun 16, 2017

There's a browser support consideration in choosing to adopt key, since it's only available in the very latest version of iOS Safari, and only has partial support in IE11:

http://caniuse.com/#feat=keyboardevent-key

@mkaz
Copy link
Member Author

mkaz commented Jun 16, 2017

@aduth ok, good point, switched back to using keyCode.

MDN docs made it seem like it was time to switch, though in this case since its the Escape key wouldn't make much difference for iOS :-)

@aduth
Copy link
Member

aduth commented Jun 16, 2017

Add event.stopPropagation() instead of default

Can you explain this change?

though in this case since its the Escape key wouldn't make much difference for iOS :-)

Bluetooth keyboards! But yeah, edge case.

@mkaz
Copy link
Member Author

mkaz commented Jun 16, 2017

Initially I had event.preventDefault() but switched it to event.stopPropagation() since I'm not trying to stop the default Esc action, but stopping the event from propagating to the above component

I want the Escape to only close the link box that this component opens and not close the block overall.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@mkaz mkaz merged commit 0bf22a4 into master Jun 16, 2017
@mkaz mkaz deleted the fix/link-escape branch June 16, 2017 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants