-
Notifications
You must be signed in to change notification settings - Fork 13
Fix: escape shortcut key overriding each other #49
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
Fix: escape shortcut key overriding each other #49
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
src/KeyCommand/index.js
Outdated
| // https://github.com/Expensify/App/issues/18480 | ||
| const strictIndex = _.findIndex(commands, item => ( | ||
| (item.input === json.input && matchesModifierFlags(item)) | ||
| ((item.input === json.input || (matchesEscape(item))) && matchesModifierFlags(item)) |
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.
This technically happens to every non single letter input, but I only added it for escape for now. Let me know if I should handle them all.
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.
I'm not sure I understand the logic here. Why are item.input and json.input not equal? (and what are their values)
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.
I'm still trying to understand the bug. Here is what I think is going: After you press ESC, we look for an exact (strict) match and we fail and fallback for loose match and we end up with the listener for SHIFT + ESC (because it's somehow first in the list). Does that sound about right? If so I think we should add a condition to handle exact ESC key combination i.e.:
const strictIndex = _.findIndex(commands, item => (
(item.input === json.input && matchesModifierFlags(item))
|| (matchesEnter(item) && matchesModifierFlags(item))
|| (matchesEscape(item) && matchesModifierFlags(item))
));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.
Ahh it's exactly the same logic your are making XD
Can you just rewrite that way as I think it's a little more readable
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.
Haha, I'm trying to keep it short. Updated.
|
cc: @s77rt for review |
|
cc @neil-marcellini please merge this |
Same root cause as Expensify/App#18480 and same solution too.
Related issue: Expensify/App#61622
Added Shift + ESC shortcut to the example app. Both ESC and Shift + ESC works fine.
w.mp4