Properly cancel keydown events after handling#344
Merged
timroes merged 3 commits intoelastic:masterfrom Feb 6, 2018
Merged
Conversation
cjcenizal
reviewed
Jan 29, 2018
CHANGELOG.md
Outdated
|
|
||
| **Bug fixes** | ||
|
|
||
| - Stop propagation and prevent default when closing components. [(#344)](https://github.com/elastic/eui/pull/344) |
Contributor
There was a problem hiding this comment.
Can we flesh this out?
- Stop propagation and prevent default when closing components. Otherwise the same Escape keypress could close the parent component(s) as well as the one you intend to close. [(#344)](https://github.com/elastic/eui/pull/344)
Contributor
|
@timroes Did you look in popover too? I think ESC is used to close that component as well. |
Contributor
Author
|
@cjcenizal you were right. I searched the whole repository, but for |
cjcenizal
approved these changes
Jan 30, 2018
Contributor
cjcenizal
left a comment
cjcenizal
left a comment
There was a problem hiding this comment.
LGTM! Did you test this and verify that it solved the original problem?
Contributor
Author
|
@cjcenizal yeah it fixes the issue. |
Contributor
|
Is this ready to be merged @timroes ? |
Contributor
Author
|
Yeah, just waiting for the second review, or can we also merge with one review in EUI? |
Contributor
|
I think one is OK, it's up to you. |
snide
approved these changes
Feb 6, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Whenever we handle a keydown event, we should properly prevent it's default behavior and also stop propagation to parent nodes. Otherwise the same Escape keypress could close multiple components.
I have the case where I have an
EuiCodeBlockin anEuiFlyout. If you now open the code block fullscreen and press Escape both components will close, because theEuiCodeBlockdoesn't stop propagation when handling its close event.That same will be true for any of these components if you place them in something else, that tries to handle the same keypress. In general: if we handle a keyevent we should prevent it's default behavior and stop its propagation.