Skip to content
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.

Sign up for GitHub

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

Enable hotkey on INPUT/SELECT/TEXTAREA elements #146

Closed
wants to merge 2 commits into from
Closed

Enable hotkey on INPUT/SELECT/TEXTAREA elements #146

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be71d26
Enable hotkey on INPUT/SELECT/TEXTAREA elements
gagandeepgill Aug 26, 2019
47e8114
Add an option to enable hotkeys for certain tags
gagandeepgill Oct 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,29 @@
import hotkeys, {HotkeysEvent} from 'hotkeys-js';
import {useCallback, useEffect} from "react";
import hotkeys, { HotkeysEvent } from 'hotkeys-js';
import { useCallback, useEffect } from 'react';

type CallbackFn = (event: KeyboardEvent, handler: HotkeysEvent) => void;
type TagNames = 'INPUT' | 'TEXTAREA' | 'SELECT';

export function useHotkeys(keys: string, callback: CallbackFn, deps: any[] = []) {
export function useHotkeys(
keys: string,
callback: CallbackFn,
deps: any[] = [],
tagNames?: TagNames[],
) {
const memoisedCallback = useCallback(callback, deps);

useEffect(() => {
// Enable hotkeys for INPUT/SELECT/TEXTAREA elements
// https://github.com/jaywcjlove/hotkeys#filter
hotkeys.filter = ({ target, srcElement }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it makes sense to enforce this specific filter implementation for only specific tag names - it's not very extendable. Imo, it would be better to allow the user to pass in their own filter implementation, so they had more customization.

If we don't want an options object passed in yet, you could have the user pass in the filter function themselves and set the hotkeys.filter if it's passed in.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the filter docs it is only used on these three tags.

By default hotkeys are not enabled for INPUT SELECT TEXTAREA elements.

I'm not sure what other custom filter option you would pass in instead of those tags

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm usually a fan of keeping things more extensible and letting end users implement how they want. In this case, if the internal filter implementation of hotkeys changes, or they add other tags that are filterable in the future, then this code would need to be maintained. But you're right, currently this would work.

I'll leave it up to @JohannesKlauss on what he thinks is best!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think allowing to pass a complete config to the actual hotkeys implementation makes a lot of sense, because it gives the end users full flexibility over how they want the hotkeys package to work.
Regarding on what this package should be I am not real sure. It started out as a tiny wrapper but people are using it extensively and I think it makes sense to give users the options to have a flexible usage if they want to.

What I am thinking about is whether we should move on with this simple implementation for a 1.5.4 release and think about a better and hotkeys complete param API for a version 2 or do a complete feature set for hotkeys manipulation in a 1.6 release without changing the params api of the hook.

What I would have in mind is for a v2 using just a typed out object as first parameter and the deps array as second parameter. This first object would also include all the hotkeys options.

Or for a 1.6 release we could keep all those params and just replace the new tagNames param with an options object for hotkeys.

I like the v2 approach more but it would be a breaking change. What do you guys think about this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I would have in mind is for a v2 using just a typed out object as first parameter and the deps array as second parameter. This first object would also include all the hotkeys options.

Or for a 1.6 release we could keep all those params and just replace the new tagNames param with an options object for hotkeys.

I personally like both of these suggestions. Adding the options now would also allow for consumers to get used to the options concept before making the breaking change, while still being able to flesh it out more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this fell off all our radars, but I just remembered it today while working on some hotkeys stuff :)

@JohannesKlauss, I sent out another PR switching to the 1.6 options implementation discussed above: #219

if (tagNames) {
const targetTagName = (target && target.tagName) || (srcElement && srcElement.tagName);
return Boolean(targetTagName && tagNames.includes(targetTagName as TagNames));
}
return false;
};
hotkeys(keys, memoisedCallback);

return () => hotkeys.unbind(keys);
return () => hotkeys.unbind(keys, memoisedCallback);
}, [memoisedCallback]);
}