Add support for keyboard shortcuts to <MenuItemLink>
#10790
Conversation
| 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
| 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>
| @@ -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?
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.