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

[feature]: ignoreEventCondition option #980

Merged

Conversation

alpinagyok
Copy link
Contributor

Hey!

Changes

  • added ignoreEventCondition option

Why

We need a bit finer control of when to ignore events. As an example, checking event.target.closest() by role/css

Wdyt? 😄

@vercel
Copy link

vercel bot commented Mar 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-hotkeys-hook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 11, 2023 5:25pm

@JohannesKlauss
Copy link
Owner

JohannesKlauss commented Mar 19, 2023

Thank you for your contribution!
I am happy to merge this, but before that, please add some tests for that feature and add its API to the API documentation. Also I am not too happy about the name of the option. Maybe something like ignoreEventWhen is a more precise naming?

@alpinagyok
Copy link
Contributor Author

Agree with the name 👍, thanks for the comment. Will add tests and docs in a moment

@alpinagyok
Copy link
Contributor Author

@JohannesKlauss, done 👌

Comment on lines 224 to 234
##### `enableOnFormTags`

```ts
enableOnFormTags: string[] // default: undefined
```

Normally we do not want a hotkey being triggered while a user types something into an input field. In some cases however
this might desirable. We can enable the callback trigger for an input tag using the following values:

`INPUT`, `TEXTAREA`, `SELECT`

Copy link
Owner

Choose a reason for hiding this comment

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

This text is now doubled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 👍

tests/useHotkeys.test.tsx Show resolved Hide resolved
@JohannesKlauss
Copy link
Owner

Thank you for your work!

@JohannesKlauss JohannesKlauss merged commit 9e0903e into JohannesKlauss:main Apr 14, 2023
1 check passed
@isti115
Copy link

isti115 commented Apr 25, 2023

Sorry to bump this already merged PR, but just out of curiosity I'd like to ask what the difference between filter and ignoreEventWhen is apart from the predicate being negated? Having both of these options only for the sake of one being affected by filterPreventDefault and the other not seems to be a bit redundant to me at first, but there might be something that I`m overlooking.

Also, if ignoreEventWhen is determined to be necessary, it should probably also be documented in the Options table of the top level README.

@alpinagyok
Copy link
Contributor Author

@isti115, I didn't know those existed 😄. but now that I look at it, there's no mention of filter in the code. maybe it's a leftover documentation? probably a question for the maintainer

@isti115
Copy link

isti115 commented Apr 28, 2023

@alpinagyok Wow, sorry for not checking thoroughly enough, you're completely right, it seems to have been removed at some point during the move to version 4 as far as I can tell from the commit history. I'll submit a PR to remove it from the documentation as well.

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

Successfully merging this pull request may close these issues.

None yet

3 participants