-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix Hotkeys for custom keyboard layouts #7659
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
base: develop
Are you sure you want to change the base?
Conversation
Generate changelog in
|
Correct comment & add testsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
1f553b8 to
afda787
Compare
Correct comment & add testsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
f4d729c to
986d476
Compare
Comment correctionsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
✅ Successfully generated changelog entries for:
Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. 📋Changelog Preview🐛 Fixes |
mm-wang
left a comment
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 think we may need a bit more documentation on the expected behavior here and what the exact change is, not just in code.
| combo: "ctrl + B", | ||
| group: "Modifier Tests", | ||
| label: "Test Ctrl+B", | ||
| onKeyDown: () => console.log("Ctrl+B pressed"), |
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.
note: I don't think this is sufficient for the documentation - there should be something in the UI denoting the keys pressed.
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.
Removed it and add more docs + an example
| verifyCombos(tests); | ||
| }); | ||
|
|
||
| it("handles alt modifier with special characters (macOS)", () => { |
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.
question: Do you have additional tests in mind for having an alternate layout and checking keys?
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.
Added some extra tests
- Test that event.key is used for regular keys (no modifier, Ctrl, Meta, Shift) - Test that event.code is used for Alt with non-ASCII characters - Test that event.key is used for Alt when character is normal ASCII - Verifies correct code vs key usage depending on modifier type
Add tests for keyboard layout support with different modifiersBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
- Renamed from HotkeyModifierTestExample to HotkeyModifierExample - Simplified to colored callouts (blue → green when triggered) - Positioned after piano example on hotkeys pages - Uses global hotkeys with group for dialog visibility - Tests modifier combinations: B, Ctrl+B, Shift+B, Alt+B, Meta+B 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Removed modifier test hotkeys from HotkeysTargetExample (redundant with HotkeyModifierExample) - Renamed TestCallout to ModifierCallout for clarity - Tests verify key vs code usage correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Clean up examples and rename componentBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Reorder style object keys to be in alphabetical order as required by the sort-keys eslint rule. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Fix lint error: sort object keys in hotkeyModifierExampleBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Fixes #6693
Checklist
Changes proposed in this pull request:
Use key for all non-alt keyboard shortcuts, to respect non-default keyboard layouts. Due to complexities around alt shortcuts, fall back to code for those. For default keyboard layout users, no behaviour changes.
Reviewers should focus on:
The piano widget in hotkeys in the docs.
Screenshot
No user facing changes.