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

[fixed] switched from KeyboardEvent.keyCode to KeyboardEvent.code #953

Merged
merged 5 commits into from
Oct 14, 2022

Conversation

robinmetral
Copy link
Contributor

Fixes #952.

Changes proposed:

Acceptance Checklist:

  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

KeyboardEvent.keyCode is deprecated. Since React 18 dropped support for IE, we don't need to fall back on it and can simply switch to the new property, KeyboardEvent.code.
package-lock.json Outdated Show resolved Hide resolved
specs/helper.js Outdated Show resolved Hide resolved
specs/helper.js Outdated Show resolved Hide resolved
@robinmetral
Copy link
Contributor Author

Sorry for the wait here, @diasbruno—pretty busy and the testing setup makes contributing less straightforward than I expected. I've followed-up on discussions above 🙂

Robin Métral added 3 commits June 24, 2022 15:25
- extend dispatchMockEvent test helper to support both keyCode and code
- duplicate two tests to cover the KeyboardEvent.code logic via escKeyDownWithCode and tabKeyDownWithCode
@robinmetral
Copy link
Contributor Author

@diasbruno as mentioned in #953 (comment) I tweaked the test helpers and added a couple of tests that cover the KeyboardEvent.code logic in some scenarios. Let me know if I should also update other test cases using the legacy test helpers.

Also FYI: locally I have ~2-4 unrelated tests failing, they seem a bit flaky. I'm running Node 14, maybe that's it? (I've seen Node 8 mentioned in some config files, but surely that's a leftover.) Anyways since it's also failing on master for me, it's unrelated to this PR

Let me know if there's anything else or if we can move forward with this 🙂

specs/helper.js Show resolved Hide resolved
specs/helper.js Show resolved Hide resolved
@diasbruno
Copy link
Collaborator

Grande lavoro! 🎉

I'll finish the review later, but it seems that everything looks great.

Thanks for the contribution.

@robinmetral
Copy link
Contributor Author

Amazing! Thanks @diasbruno 🙂

@robinmetral
Copy link
Contributor Author

@diasbruno just checking in 🙂 let me know if there's something else I can do on my side!

@robinmetral
Copy link
Contributor Author

Tumbleweed gif

@robinmetral
Copy link
Contributor Author

robinmetral commented Oct 14, 2022

(PS. no offence meant at all, just stumbled upon the issue again and I wanted to check if there's anything we can do to bring the change over the line 🙂 but totally understand that it can take some time)

@diasbruno
Copy link
Collaborator

@robinmetral Thanks for pinging me...It's been hard to find time to manage this repo.
Today I have another PR to merge, so I'll add this one to release a new version.

@diasbruno diasbruno merged commit 840a0f6 into reactjs:master Oct 14, 2022
@diasbruno
Copy link
Collaborator

Done. @robinmetral sorry for taking so long...:joy:

@robinmetral robinmetral deleted the keyboardevent.code branch October 15, 2022 07:54
@robinmetral
Copy link
Contributor Author

No worries at all, and thanks a lot for the review and release! 🙌

@diasbruno
Copy link
Collaborator

@robinmetral Just released version 3.16.1. Thanks for taking the time to work on this.

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.

Switch from KeyboardEvent.keyCode to KeyboardEvent.code
2 participants