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.

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

Filters seem scoped, but aren't #276

Closed
pimterry opened this issue May 18, 2020 · 7 comments
Closed

Filters seem scoped, but aren't #276

pimterry opened this issue May 18, 2020 · 7 comments

Comments

@pimterry
Copy link

An example:

useHotkeys('Delete', () => {
    // do something
}, { filter: ignoreInputElements });

useHotkeys('Ctrl+Shift+Delete', () => {
    // do something
}, { filter: () => true });

You might want to have different filters for different shortcuts. Here, for example, 'Delete' is something you might press during input, so needs to be filtered to ignore input elements, but 'Ctrl+Shift+Delete' is not.

Unfortunately, the hotkey filter functionality is global, so the last filter that's set always wins. In the above example, 'Delete' will fire everywhere, even in input elements, which is not what the author intended.

This isn't very react-y/declarative, as different render orders/rerenderings elsewhere can produce all sorts of strange behaviours here. It'd be better to either scope the filters to the specific shortcut somehow, or to make this behaviour obvious in the useHotkeys API. Setting the filter via useHotkeys.filter = ... would make the fact that it's global much clearer, for example.

@JohannesKlauss
Copy link
Owner

I'd love to scope it, but i am not sure if this is possible with the underlying hotkeys package. Maybe it's time to think about a custom implementation. But this would take a while

@louisrli
Copy link
Contributor

+1. I ran into this issue and was looking through the code, I can see that hotkey.filters is set globally. As far as I can see from the original library, they don't let you use filter per call to hotkey, so it might not be possible, but it would be nice if this were at least documented in the options of the hook (that filter is not isolated between hook usages. same with enableOnTags, since its impl just uses filter)

louisrli added a commit to louisrli/react-hotkeys-hook that referenced this issue Jun 12, 2020
f-ricci added a commit to f-ricci/react-hotkeys-hook that referenced this issue Jun 15, 2020
This fixes the issue of globally scoped filters
@JohannesKlauss
Copy link
Owner

I finally have a working dev machine again. I am going through the issues in the next few days. Bare with me.
Regarding the scoping of the filters, this will only be possible if we use either a custom hotkeys version or build our own. I'll think about this. It probably would be nice to have a independent solution for this so we can add custom features if we want to.

@JohannesKlauss
Copy link
Owner

So I was thinking about this, but I just don't have the time to come up with a custom all browser compatible solution for a hook based hotkeys package. If anyone else wants to create something like this I'd be glad to support where I can. But for now I just can't make the time.

@SteffeyDev
Copy link

I found a fairly easy fix that I implemented in my fork (https://github.com/SteffeyDev/react-hotkeys-hook/blob/feature/scope/src/useHotkeys.ts). Can't PR though cause I made a lot of other changes in my fork that ended up not being good because of how poorly hotkeys-js handles scoping.

The idea is that you set the global hotkeys.filter to () => true so that all events are allowed through. Then you do all the filtering in the hook. The default filtering that hotkeys uses is fairly simple to copy over if the user does not pass a filter in the hook, but if they do you can use their filter instead. Once again, see the version in my fork to see it implemented in a working fashion (just ignore all my scope changes that don't work well).

If someone wants to fork and implement, it should be pretty easy. Unfortunately this would be a breaking change so would probably need a new major version.

@JohannesKlauss
Copy link
Owner

Thank you for sharing, I'll have a look and implement it.

@JohannesKlauss
Copy link
Owner

I implemented a custom filter logic.

Basically everything works as before, but the filter is now scoped to each hook instance instead of globally.

Since this is a behavioral breaking change, I bumped the major version to 3.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants