Skip to content

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented May 22, 2025

After upgrading to 5.1.0, my > hotkey stopped working, because of the new sequential combo feature (#1247). (So, ideally, the sequential combo feature should have been marked as a breaking change; oh well.)

Here are some suggestions for improving this:

  • Add documentation for the new options to the README and the API. (Currently, it looks like it's only documented in the basic usage section.)
  • Add a validation check for apparently invalid key parses. (> is parsed as a sequence of ['', ''], which indicates a problem.) I'd appreciate a review of my validation logic here, to make sure there aren't any valid use cases that trigger my logic, but it at least doesn't trigger on any of the automated tests.

I have not added a test for the new validation check. If you'd like for me to do that, please let me know.

Within the updated documentation, I'm using relative links rather than absolute links, to help the docs work better whenever 6.x is released and 5.x is moved to https://react-hotkeys-hook.vercel.app/docs/4.x/. Please let me know if you'd like for me to update other links to be relative or if you'd rather I'd use absolute links.

It occurred to me that react-hotkeys-hook could probably assume that any standalone delimiter, splitKey, or sequenceSplitKey could be treated as a literal - but "magic" behavior of that sort may cause more problems that it solves.

Copy link

vercel bot commented May 22, 2025

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 May 22, 2025 9:49pm

@JohannesKlauss
Copy link
Owner

JohannesKlauss commented May 23, 2025

You are absolutely right about the breaking change, I missed that. I think a better fix would be to check if a single > is present. If the parsed sequence has a length < 2, it should not be treated as a sequence. This will prevent accidental sequences like shift+>, >, >+1, etc. Could you add this to the PR?

@joshkel
Copy link
Contributor Author

joshkel commented May 26, 2025

I think a better fix would be to check if a single > is present. If the parsed sequence has a length < 2, it should not be treated as a sequence.

I may be misunderstanding this suggestion. If > is present, then the parsed sequence will always have a length < 2. E.g., '>'.split('>') results in ['', ''].

As far as I know, the validation logic that I introduced (singleCharKeys.some(i => !i)) will detect issues like this. I can use that if you like (and change it to "don't treat as a sequence" rather than "treat as a sequence but log a warning").

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.

2 participants