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

Reset resolution handler state on global hotkeys enable calls #157

Closed
wants to merge 1 commit into from
Closed

Reset resolution handler state on global hotkeys enable calls #157

wants to merge 1 commit into from

Conversation

mrlubos
Copy link

@mrlubos mrlubos commented Mar 23, 2019

No description provided.

@mrlubos mrlubos closed this Mar 25, 2019
@mrlubos mrlubos deleted the bugfix/150-global-hotkeys-dom branch March 25, 2019 09:45
@StephenHaney
Copy link

Hey @mrlubos - is this PR the same you ended up using in your fork? Would you still recommend that it go into the library?

@mrlubos
Copy link
Author

mrlubos commented Apr 19, 2019

Hi @StephenHaney, yes! I have done a bit more work on this to also generate shortcut maps (the master generates only active shortcuts whereas I wanted all available shortcuts), though there are still issues I'd like to see fixed (e.g. disabling the submatch feature). I can publish the fork if that would be of use at Modulz, I am watching your progress! (I also applied for a role with you a few months ago 😀)

@StephenHaney
Copy link

Hey @mrlubos that's so cool! What are the odds? Definitely keep in touch, we'll be hiring again in the future!

We've been using this library and really like it, so we're going to try and help fix up some things too and hopefully push back into this repo.

We tried out this PR and it fixed #150 for us, verified by @hadihallak. Would you mind re-opening this PR as a fix for #150 if you agree that it's a good contribution to the library?

@greena13 let me know if this makes sense to you too.

@greena13
Copy link
Owner

greena13 commented Apr 24, 2019

Hey @StephenHaney, yes this particular PR is tabled for inclusion in the master branch. I assume you've read through the discussion on #150.

I would prefer for this PR to be re-opened so I can merge and give @mrlubos the proper credit for the fix, otherwise I will be simply copying it across, and revising as necessary.

I am also eager to hear more about the displaying all keyboard shortcuts. This indeed sounds useful.

@mrlubos
Copy link
Author

mrlubos commented Apr 24, 2019

@greena13 Regarding keyboard shortcuts, the first step was to add meta data to shortcuts as described in #154.

const hotkeys = {
  group: 'group',
  keys: [
    {
      description: 'this shortcut does amazing stuff',
      key: 'key1',
      sequence: 'a',
    },
    {
      hidden: true, // shortcut will be hidden in generated list view, no description necessary
      key: 'key2',
      sequence: 'b',
    },
  ],
};

In addition to that, I introduce a different approach to creating shortcut maps. We start by having an array including all shortcuts available in our application.

const allHotkeys = [hotkeys];

This array is passed to the function which generates list of all available shortcuts. This way, we ensure we do not accidentally miss some shortcuts.

import { getAllHotKeys } from 'react-hotkeys';

getAllHotKeys(allHotkeys);

This method works pretty much the same as the existing method for generating key maps, except it accepts different arguments. As you saw above, we can for example prevent certain sequences from being returned in the result. (I also added a support for tooltip hints.)

Since we now have hotkeys inside this object above, we needed a helper method to transform them into a parameter react-hotkeys recognises.

const createKeyMap = keys => keys.reduce((keyMap, action) => {
  keyMap[action.key] = action.sequence;
  return keyMap;
}, {});

const keyMap = createKeyMap(hotkeys.keys); // your good ol' key map

// later in your component

<GlobalHotKeys keyMap={keyMap} />

The last step (which I am yet to do) is to require the parent-child relationship between allHotkeys and hotkeys. This can be done as follows:

  1. Configure react-hotkeys on launch.
import { configure } from 'react-hotkeys';

configure({
  hotkeys: allHotkeys,
});
  1. Any component will check if the keyMap prop is a child of configure.hotkeys. If not, we throw an error. This rule can be relaxed in configure.strict to make this completely backward-compatible.

  2. Generate list of key maps! 🎉

import { getAllHotKeys } from 'react-hotkeys';

getAllHotKeys(); // our shortcuts list

This approach does not make it possible to change the main shortcuts array on the fly, but that's the idea, your shortcuts list should not update dynamically. (I don't know of an example when this would be desirable.)

Hope this helps!

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.

3 participants