Enable hotkey on INPUT/SELECT/TEXTAREA elements

[gagandeepgill]

fixes #127

Reference: https://github.com/jaywcjlove/hotkeys#filter

[JohannesKlauss]

Hmh. Thank you for your PR, but this enables the filter by default. I don't think that this is a desired behavior for all users of this library. Maybe we could add a parameter for this?

[hmafzal]

@gagandeepgill did you have a change ready to update this with? I was seeing this issue as well and would be interested in the functionality.

Going off @JohannesKlauss suggestion, I think you could add a parameter like options which has an optional filter prop. If the filter exists, then you could override it on hotkeys:

export function useHotkeys(keys: string, callback: CallbackFn, deps: any[] = [], options: {} = {}) {
...
  useEffect(() => {
    if (options.filter) hotkeys.filter = options.filter;

[JohannesKlauss]

Sounds good to me for compat reasons. In a v2 I'd suggest making the parameters more compact. A hook with 4 parameters, one being an object seems a bit overkill to me.

@hmafzal Maybe naming the option filter isn't the best idea. I think we could give it a more expressive name for users that are not that familiar with hotkeys.js

[gagandeepgill]

@JohannesKlauss @hmafzal I have updated the code to allow options to be passed in

[dan-lee]

@JohannesKlauss

Sounds good to me for compat reasons. In a v2 I'd suggest making the parameters more compact. A hook with 4 parameters, one being an object seems a bit overkill to me.

As I definitely agree with you, I would love to pass some options down to the actual hotkeys implementation (in my case I need to set keyup: true). Having only very specific options enabled eventually leads to even more params in the hook.

[JohannesKlauss]

I think this feature should also be considered as v2 and the hotkeys options param be part of it.

[hmafzal]

@JohannesKlauss I think it just depends on how you want to treat this package - whether it is meant to be a lightweight hook wrapper around hotkeys or it will have more options with additional functionality (and documentation). As of now, it seems like it's meant to be lightweight, in which case I'd argue that maybe filter would work. Anyone using this package currently would need to know how to use hotkeys anyway, so I think keeping naming consistent would make it easier to understand what option does what (and would allow you to upkeep less documentation on this package). But I think that would only work if you made it clear to users to defer to hotkeys for documentation!

[hmafzal]

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.

[gagandeepgill]

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

[hmafzal]

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!

[JohannesKlauss]

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?

[hmafzal]

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.

[hmafzal]

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

[JohannesKlauss]

@gagandeepgill I finally implemented this. Sorry for taking way too long. Since everything changed a little bit with v2 I just copied your changes over. Thank you for your work :)