-
Notifications
You must be signed in to change notification settings - Fork 3
Add useKeyboardShortcut hook #368
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: main
Are you sure you want to change the base?
Conversation
889dd4b to
a94bab5
Compare
a94bab5 to
0287617
Compare
| export function useEventListener<T extends EventTarget>( | ||
| targetOption: RefObject<T> | T | null | undefined, | ||
| export function useEventListener<T extends EventTarget | null>( | ||
| targetOption: Unreffable<T>, |
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 using Unreffable<T> and adding <T extends EventTarget | null> is the correct way to support refs that can be null.
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 created a separate index.ts file to export more things. Not sure if we want to export all the types in the package index, or include that file as a separate export in the package.json?
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.
Not sure if this will be useful as a separate top-level hook, since it's set up quite generic already?
|
Wouldn't it be much simpler to replace the key options with something like https://github.com/ianstormtaylor/is-hotkey? |
That library hasn't been updated in over 5 years, and uses legacy keycodes (numbers) instead of new codes https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_code_values Also, it's not typed at all. But happy to update parts of the hook if you find a better alternative :D |
| // For "auto" | ||
| // - when we have Ctrl/Cmd modifiers, the shortcut is safe to use | ||
| // - otherwise, we ignore the shortcut when in an input, | ||
| // as it will also type the character in the input when wanting to trigger the shortcut | ||
| const ignoreWhenInputFocused = | ||
| ignoreWhenInputFocussedProp === 'auto' | ||
| ? !hasInputSafeModifiers(shortcut) | ||
| : ignoreWhenInputFocussedProp; |
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.
Should be defined in useEventListener handler
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.
Why? It doesn't change in between keypresses.
It should probably be memo'd.
| if ( | ||
| ignoreWhenInputFocused && | ||
| (target.isContentEditable || | ||
| target instanceof HTMLInputElement || | ||
| target instanceof HTMLTextAreaElement || | ||
| target instanceof HTMLSelectElement) | ||
| ) { | ||
| // Ignore shortcut when inside an input. | ||
| return; | ||
| } |
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 don't think this check should be part of this hook, it should be handled by the implementation.
useKeyboardShortcut("mod+a", () => {
if (
document.activeElement.isContentEditable ||
document.activeElement instanceof HTMLInputElement ||
document.activeElement instanceof HTMLTextAreaElement ||
document.activeElement instanceof HTMLSelectElement
) {
return;
}
// Handle interaction if element is not an input
})Maybe we can export a util for the condition so that people can re-use it.
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.
Firstly, this behaviour is something you want 99% of the time, so it would be cumbersome for users to have to add this to event handlers every time.
Secondly, if they forget to add it, it would result in triggering shortcuts and potentially preventing the input when in input fields (not what you want).
Thirdly, that logic should only be executed when ignoreWhenInputFocused is true, and this value is dependent on the shortcut itself when the option is set to auto (default behaviour).
This means that this field should also be passed to the callback handler, which the user has to make sure to forward to the input-checking util. Which makes this even more cumbersome.
And at that point is weird that there is some logic (the 'auto' shortcut checking) part of the hook, while not used internally at all. But if you also move that outside, you have to either forward or re-type the shortcut (so it can be detected).
All in all, while I agree with composition in general, in this case this logic is too core to the small surface area of this hook, and pulling it out makes configuring (using) the hook way too cumbersome, without too much added value.
| * Keyboard event code values based on MDN documentation | ||
| * @see https://developer.mozilla.org/en-US/docs/Web/API/UI_Events/Keyboard_event_code_values | ||
| */ | ||
| export const keyCodes = { |
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.
This is quite a bit of code to add, could we make this a const enum instead? Not sure if that works with a build package 🤔
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.
This const is not used by the hook itself, so will only become part of the app bundle when the user explicitly imports it.
Internally it's only used for typing. And exported as a convenience in case the user likes to make use of it.
| @@ -0,0 +1,210 @@ | |||
| /* eslint-disable @typescript-eslint/naming-convention */ | |||
|
|
|||
| export const modifierKeys = { | |||
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.
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.
Modifiers are not that sizable, and its values are used runtime in code (in Object.values()). Swapping them out with const enums would make the code less dynamic and potentially even larger.
| windowsCommand?: Shortcut; | ||
| macOsCommand?: Shortcut; |
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.
What about other platforms? The is-hotkey package has a custom mod identifier that acts the same across platforms, I like that approach.
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.
For this I'm auto-converting cmd to ctrl on windows. However, this means that you cannot use the windows key on windows with this behaviour enabled – not sure if you ever want to make use of the windows key in web apps, but still.
Supporting mod as a generic shortcut might indeed be a nicer option.
Has more options/features than just that, see docs for all use cases:
https://github.com/mediamonks/react-kit/blob/eac053eab37049de4e0c35c4d5005df36a7412fe/src/hooks/useKeyboardShortcut/useKeyboardShortcut.mdx
Also contains a typed object of all the KeyCode names.
mod, and remove theconvertPlatformsoption