Add support for keyboard shortcuts to <MenuItemLink>#10790
Conversation
Madeorsk
left a comment
Madeorsk
left a comment
There was a problem hiding this comment.
Great work! I love this feature!
| className, | ||
| hasDashboard: hasDashboardProp, | ||
| ...rest | ||
| } = props; |
There was a problem hiding this comment.
If you just want to discard hasDashboard, what about using _ instead of hasDashboardProp?
There was a problem hiding this comment.
That's how we usually do it
fzaninotto
left a comment
fzaninotto
left a comment
There was a problem hiding this comment.
That's a good start but you should also add a general "keyboard shortcut" chapter to make it discoverable through search ,explaining how to add shortcuts for menu items and for custom actions.
Also, could you please add shortcuts to the simple example menu ?
| if (open && keyboardShortcut != null) { | ||
| return ( | ||
| <Tooltip | ||
| title={getKeyboardShortcutLabel(keyboardShortcut)} |
There was a problem hiding this comment.
I'm not fond of the way we surface these shortcuts:
- They only display on hover,
- They only display when the sidebar is open
Why not show them on the right side of the MenuItemLink instead? It's common practice.
There was a problem hiding this comment.
I don't know how to represent sequential shortcuts like this
There was a problem hiding this comment.
We decided to just add a space between all keys, sequential or not
| useHotkeys, | ||
| } from 'react-hotkeys-hook'; | ||
|
|
||
| export const KeyboardShortcut = (props: KeyboardShortcutProps) => { |
There was a problem hiding this comment.
please add jsDoc to explain why we need that (to add a keyboard shortcut conditionally? I so, why don't we use the "enabled" option?)
| export const getKeyboardShortcutLabel = (keyboardShortcut: Keys) => { | ||
| if (typeof keyboardShortcut === 'string') { | ||
| return keyboardShortcut.split('+').join(' + '); | ||
| } | ||
| return keyboardShortcut | ||
| .map(shortcut => getKeyboardShortcutLabel(shortcut)) | ||
| .join(', '); | ||
| }; |
There was a problem hiding this comment.
This is a bit short, we'd need to be smarter than that and show icons for key modifiers dependent on the platform (e.g. command / windows)
If you don't mind, I'll add this new page when we'll have more components supporting keyboard shortcuts (buttons, etc.) |
Co-authored-by: Jean-Baptiste Kaiser <jb@marmelab.com>
fzaninotto
left a comment
fzaninotto
left a comment
There was a problem hiding this comment.
Awesome user experience! I even tested the shortcuts when editing a fom, and the hook is smart enough not to trigger them in this case.
| @@ -0,0 +1,119 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
Seeing this file at the root of src, it seems to have a bigger importance that it actually has. I'd put this component (and its story) under the layout directory.
There was a problem hiding this comment.
You remember that we will probably use it for buttons too?
4 participants
This PR adds support for keyboard shortcuts to
<MenuItemLink>How To Test
https://react-admin-storybook-git-menu-item-keyboard-shortcuts-marmelab.vercel.app/?path=/story/ra-ui-materialui-layout-menu--with-keyboard-shortcuts
Additional Checks
masterfor a bugfix or a documentation fix, ornextfor a featureAlso, please make sure to read the contributing guidelines.