-
Notifications
You must be signed in to change notification settings - Fork 172
Migrate out of react-hotkeys to @mantine/hooks #2968
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
Conversation
161c3d6
to
b70fa5f
Compare
Pull Request Test Coverage Report for Build 8934209129Details
💛 - Coveralls |
b658b16
to
fac3eda
Compare
fac3eda
to
b63fd75
Compare
Note that we plan to shift DataViz to a module: #2809 |
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.
Thanks for this! Did a quick run through and some comments below:
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.
LGTM, thanks!
const hotkeyBindings: HotkeyItem[] = this.state.visualization | ||
? [ | ||
['a', this.stepFirst], | ||
['f', this.stepNext], | ||
['b', this.stepPrevious], | ||
['e', this.stepLast(this.props.stepsTotal)] | ||
] | ||
: [ | ||
['a', () => {}], | ||
['f', () => {}], | ||
['b', () => {}], | ||
['e', () => {}] | ||
]; |
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.
While not related to this PR, it it just be or are the keyboard shortcuts not intuitive? I would expect the following:
(first, prev, next, last) to be mapped to (h, j, k, l) or (a, s, d, f) -- basically just a sequence of keys that are next to each other on the keyboard.
Description
Consolidating our usage of 3rd party hooks under the mantine hooks library.
h u l k
hotkey sequence has been replaced withalt+shift+h
since mantine'suseHotkey
does not support hotkey sequencesCloses #2935
Type of change
How to test
Checklist