⌨️ Keyboard shortcuts/navigation for create and upload #736
Conversation
MontaGhanmy
commented
Nov 20, 2024
MontaGhanmy
commented
| // translate shortcuts | ||
| const translateDynamicShortcut = (shortcut: string): string => | ||
| shortcut.startsWith('CmdOrCtrl+Shift') | ||
| ? `⇧⌘${shortcut.replace('CmdOrCtrl+Shift+', '')}` |
There was a problem hiding this comment.
this symbol means nothing to non mac users =/ i think we should use https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform to show control if it isn't mac
| : shortcut; | ||
|
|
||
| if (props.shortcut) { | ||
| useEffect(() => { |
There was a problem hiding this comment.
i think the if (shortcut) should be inside the useEffect callback, because useEffect and friends need to always be called in the same exact order and sequence
| return () => { | ||
| removeShortcut({ | ||
| shortcut, | ||
| handler: event => { |
There was a problem hiding this comment.
This function instance will not be the same as the one when added. If the event source tests for the handler it will fail, if it doesn't, this is useless
| @@ -58,6 +58,46 @@ export const Button = (props: ButtonProps) => { | |||
| else className = className + ' w-9 !p-0 justify-center'; | |||
| } | |||
|
|
|||
| // translate shortcuts | |||
| const translateDynamicShortcut = (shortcut: string): string => | |||
There was a problem hiding this comment.
This function should probably be in app/features/global/services/shortcut-service
| props.onClick && props.onClick({} as any); | ||
| }; | ||
|
|
||
| const shortcut = props.shortcut || ''; |
There was a problem hiding this comment.
this is inside if (props.shortcut) so it's always truthy here
| return ( | ||
| <div | ||
| onClick={props.onClick} | ||
| className="flex flex-row p-4 dark:bg-zinc-900 dark:text-white bg-zinc-100 hover:bg-opacity-75 cursor-pointer rounded-md m-2" | ||
| className="flex flex-row p-4 dark:bg-zinc-900 dark:text-white bg-zinc-100 hover:bg-opacity-75 cursor-pointer rounded-md m-2 focus:bg-zinc-800 dark:focus:bg-zinc-800 outline-none focus:border-none" | ||
| tabIndex={0} |
There was a problem hiding this comment.
why the {} and not just 0 ? is this required at all, we're not really doing the tab indexing stuff anywhere else ?
There was a problem hiding this comment.
tabIndex is a number that's why. It is required for tab to work/navigate the modal items.
There was a problem hiding this comment.
html dom attribute values are always strings anyway, see https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute . what's happening here is just making react type it first and maybe not guess its a constant it could optimise out
| @@ -162,6 +162,7 @@ export default () => { | |||
|
|
|||
| <Button | |||
| onClick={() => uploadItemModal()} | |||
| shortcut='CmdOrCtrl+U' | |||
There was a problem hiding this comment.
is the modifier necessary ?
