-
Notifications
You must be signed in to change notification settings - Fork 400
fix(tooltip): handle esc keydown event #2164
fix(tooltip): handle esc keydown event #2164
Conversation
Deploy preview for carbon-components-react ready! Built with commit 78c922b https://deploy-preview-2164--carbon-components-react.netlify.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 - Thanks @dakahn!
src/components/Tooltip/Tooltip.js
Outdated
@@ -374,6 +374,11 @@ class Tooltip extends Component { | |||
handleKeyPress = evt => { | |||
const key = evt.key || evt.which; | |||
|
|||
if (key === 'Escape' || key === 27) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (key === 'Escape' || key === 27) { | |
if (key === 'Escape' || key === 'Esc' || key === 27) { |
Talking with @vpicone revealed that evt.key
yields Esc
in IE and looks that we should cover that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would we want to also update and use these helper functions https://github.com/IBM/carbon-components-react/blob/master/src/tools/key.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emyarod Good point - I think it makes sense to me.
…onents-react into fix/2076-tooltip-a11y
Thanks @dakahn for the update and my apologies for being late on this - Updated code basically looks good to me, could you please look at the test failure? |
The updated code LGTM 👍 - Thanks @dakahn! |
adds a check in the
handleKeyPress
method that closes the tooltip if escape is pressed.ref #2076
to test
open the tool tip and press escape. the tooltip should close.