Simplify key codes expression in Tags component #4248
Conversation
|
|
stesel
changed the title
There was a problem hiding this comment.
Hi @stesel thanks for the code! The PR looks nice.
A question: would you be willing to go a step further and switch from event.keyCode, which is deprecated, to use event.key, see https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key
| e.keyCode != KEYS.RIGHT && | ||
| e.keyCode != KEYS.TAB | ||
| ) { | ||
| } else if ((e.keyCode < 65 || e.keyCode > 90) && !COMBO_KEYS.includes(e.keyCode)) { |
There was a problem hiding this comment.
Could we line wrap this conditional? It's just a bit long for reading on one line in my opinion. 😬
Sure, I've migrated to event.key. Now I have questions, do we need fallbacks for old browsers witch doesn't support |
|
@stesel I don't think so, I'm looking at both https://caniuse.com/#feat=keyboardevent-key and https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent#Browser_compatibility and I think we're fine. It's obviously open to discussion... |
|
@rhymesfor thank you for response! |
pr-triage
bot
added
PR: reviewed-approved
pr-triage
bot
added
PR: merged
* Simplifying this key codes expression in Tags component * Refactor KeyboardEvent keyCode to key * Simplify cyrillic letter regexp * Wrap condition in 2 lines
What type of PR is this? (check all applicable)
Description
Move combination key codes to separate const in
tags.jsxfile.Add test for checking
preventDefaultinhandleKeyDownmethod.Suggestions comes form codeclimate.
https://codeclimate.com/github/thepracticaldev/dev.to/app/javascript/shared/components/tags.jsx/source#issue-a087ee1bf368783e621a0b4563a3aaeb
Related Tickets & Documents
https://codeclimate.com/github/thepracticaldev/dev.to/app/javascript/shared/components/tags.jsx/source#issue-a087ee1bf368783e621a0b4563a3aaeb
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?
[optional] What gif best describes this PR or how it makes you feel?