-
Notifications
You must be signed in to change notification settings - Fork 844
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
use KeyboardEvent.keys
instead of KeyboardEvent.keyCodes
#3517
use KeyboardEvent.keys
instead of KeyboardEvent.keyCodes
#3517
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
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.
Code changes themselves look great, one request on the changelog.
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3517/ |
Still want this in draft? |
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.
Changes LGTM; Re-requested Dave's input on the PR description.
Another jenkins run after the merge with master, will merge if passing. jenkins test this |
Looks like master introduced a new dependency on the refactored service. |
f45d674
to
78cb2b1
Compare
@chandlerprall all rebased and an update for ToolTip is in 78cb2b1 |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3517/ |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3517/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3517/ |
thanks! now that this has merged I updated the PR description, as planned, with links to relevant source code to make it as easy as possible for consumers to update. |
Summary
closes #3512
This PR suggests removing usages of KeyboardEvent.keyCode (since it's deprecated) in favor of its successor KeyboardEvent.key.
Checklist
Breaking Change Info
What changed?
From
@elastic/eui/lib/services
, the following have been renamedkeyCodes
key
cascadingMenuKeyCodes
cascadingMenuKeys
comboBoxKeyCodes
comboBoxKeys
In addition to renaming, the values of
keys
,cascadingMenuKeys
,comboBoxKeys
, as well asaccessibleClickKeys
have also been updated to follow theKeyboardEvent.key
API, rather than theKeyboardEvent.keyCode
API.For example,
cascadingMenuKeyCodes.ESCAPE
27
cascadingMenuKeys.ESCAPE
'Escape'
accessibleClickKeys.UP
38
accessibleClickKeys.ARROW_UP
'ArrowUp'
What specific steps do I take?
KeyboardEvent.key
. The an up-to-date comparability graph can be found on the mdn specification page, but the following represents support level as of May 2020:keyCode
to catch examples where anevent.keyCode
is accessed and replacekeyCode
usages withkey
usages. Also look forkeyCodes.
to find prior usages of thekeyCodes
constants map. Here are some examples:keyCodes
constant was used the refactor is relatively straightforward.KeyboardEvent.keyCode
is deprecated but not yet removed in any browser, any test that was previously testing forkeyCode: <keyCode numeric value>
should now also work withkey: "<key string value>"
.Things to watch out for
keyCodes
where not perfectly preserved. TheKeyboardEvent.keys
API usesArrowUp
, and so (to match that API), theUP
constant is nowARROW_UP
..keyCode
or.key
APIs and update them to use thekeyCodes
map. Presently TypeScript does not do any type checking or completion on these values (although I'm working to fix that in both the official react types as well as the official TypeScript DOM types) and it's a good practice to use the constants to avoid scenarios like:These situations are generally not terribly hard to find because somewhere in the preceeding code will be a call to
event.key
orevent.keyCode
, so searching for.key
and.keyCode
should turn them all up.