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

Reader | Centralize Keyboard Shortcuts #3977

Closed
dannysellers opened this issue Nov 22, 2017 · 1 comment
Closed

Reader | Centralize Keyboard Shortcuts #3977

dannysellers opened this issue Nov 22, 2017 · 1 comment
Assignees
Labels
Priority: Low Reported issue, not a blocker. "Last in" priority for new work. Product: caseflow-reader Team: Whiskey Type: Enhancement Enhancement to an existing feature

Comments

@dannysellers
Copy link
Contributor

dannysellers commented Nov 22, 2017

Reader is at a point where we should begin centralizing keyboard shortcuts, such that we can redirect built-in key behavior, trigger different behavior based on what element has focus, etc. (see #3824)

The solution should address two issues: Dispatching an action based on what element has focus, and stacking callbacks. Focus may be defined as document.activeElement, or simply as the component that bound a callback last (i.e. a modal).

Some relevant implementations include:

  • Creating a <ShortcutProvider/> wrapper with its own state that updates its children when a key combo has been pressed, and when it's been handled
  • Using react-hotkeys, a mixin through React.createClass()
  • Creating a class factory/decorator function to mimic mixing in a shortcut handler / integrate with a global manager. The example relies on keyboardjs.
  • Using shortcutJS, which creates a singleton shortcut manager, with components able to create and subscribe to actions dispatched on pressing a key combo.
  • Mousetrap, a pre-ES6-style module which is functionally similar to shortcutJS, but doesn't support stacked callbacks (i.e. open search bar, open Modal, press Escape to close modal, and Escape no longer closes the search bar).

The decorator and shortcutJS seem like our best options at this point: the <ShortcutProvider/> seems like it would get complicated quickly with multiple callbacks per key combo (and can only trigger behavior on children it wraps), react-hotkeys doesn't support ES6 classes (which don't support mixins), and Mousetrap only supports one callback per combo (uses Objects instead of Sets).

The main issue I found with the decorator / keyboardjs approach is that it's unclear how the events are bound—oftentimes we want a handler to respond even if its creator isn't the active element, but shortcuts didn't fire reliably even when keyboardJS.watch() was run, which is supposed to bind shortcuts to window.

After the maintainer of KeyboardJS rejected a PR to modernize the project and update it to ES6, the author of that PR created ShortcutJS. ShortcutJS seems promising, though it does have a few issues: debugging is difficult, the docs are sparse, and it doesn't support Escape at the moment (used to hide the search bar).

PRs pending on ShortcutJS:
fix(EventProcessor): Clean combo after triggering shortcut with Cmd
feat(KeyContainer): Add Escape support to KeyContainer
feat(KeyContainer): Add Tab support
feat(EventProcessor): Add option to reverse Action processing
feat(Shortcut): Add removeAction function

@dannysellers dannysellers self-assigned this Nov 22, 2017
@jimruggiero jimruggiero added the Priority: Low Reported issue, not a blocker. "Last in" priority for new work. label Sep 8, 2019
@D-L-Ware
Copy link

Closed as part of clean up of Reader tickets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low Reported issue, not a blocker. "Last in" priority for new work. Product: caseflow-reader Team: Whiskey Type: Enhancement Enhancement to an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants