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

Replace the KeyboardBinder with hooks #4937

Merged
merged 59 commits into from
Sep 28, 2022

Conversation

gatzjames
Copy link
Contributor

@gatzjames gatzjames commented Jul 7, 2022

This aims to fix many issues with the keydown bindings in the app.

Decision: user defined app hotkeys will always override code editor hotkeys, except showAutocomplete, eg. Cmd+d will duplicate request always never delete line
autocomplete is specific to code editor
If someone wants to use a code mirror hotkey they can rebind our default hotkey

There is now a more explicit way to register keyboard shortcuts and define the elements they will be attached to.
Why the createKeyboardHandler method?

  • Handles cases where we check if e.g. keyCode === 'Enter' but we incorrectly also handle 'Cmd+Enter'/'Ctrl+Enter' etc.

Class Components that would make the transition easier if they were Functions instead:

  • modal.tsx
  • graphql-editor.tsx
  • graphql-explorer.tsx
  • request-switcher-modal.tsx
  • app.tsx

In scope

  • dropdown toggles
  • dropdown closes on esc
  • modal closes on esc
  • gql editor fetches introspection query with axios
  • send and focus request types
  • adds tinykey library in order to improve readablility of binding logic
  • fixes initialValue typing issue
  • types screaming snake case hotkey names
  • codeMirror ignores use defined hotkeys
  • simplifed modal.tsx to prepare for conditional rendering approach
  • special cases enter and esc in a few components
  • eliminates the tree walking in modal.tsx
  • add key combo modal fc
  • simplify hotkeyDefs object and type KeyboardShortcut
  • remove the close modal and close dropdown bindings

Out of scope

  • examples of a useKeyboardShortcut bound to a specific dom element rather than document.body
  • conditionally rendered modals
  • decouple import export features from redux

Also closes

Closes #4766
Closes INS-1573
Closes INS-1187

changelog(Fixes): Fixed a number of issues regarding hotkey shortcuts by replacing the KeydownBinder with hooks

@daniel-stoneuk
Copy link
Contributor

I have just been looking into a bug which occurs when you use a keyboard shortcut while a dropdown is open on the develop branch.

Screenshot 2022-07-25 at 20 33 10

These lines appear to prevent keyboard shortcuts while a dropdown is open, though they are in an event handler that is called after the KeydownBinder in app.tsx so fail to do so.

// Catch all key presses (like global app hotkeys) if we're open
event.stopPropagation();

I found this PR and I wanted to mention this behaviour here in case some sort of zIndex-style priority should be built into this hook to allow components to stop event propagation. It is reproducible in gatzjames:fix/keybindings-hook.

If this is out of the scope of this PR, I can create an issue and tackle it once this has landed!

@gatzjames
Copy link
Contributor Author

@daniel-stoneuk thanks a lot for mentioning this. It's definitely in the scope and has to do with global shortcuts listening to keyboard events in the body or using the capture phase.

The purpose of this PR is to remove the keyboard binder and make it simpler to reason about keyboard events in the app.

In order to simplify the changes we decided to move as much of the affected components to function components first (remaining list on the pr comment).

Can't promise anything but the plan is to get this merged before the next release 😄

@jackkav jackkav marked this pull request as ready for review September 28, 2022 09:54
Copy link
Contributor Author

@gatzjames gatzjames left a comment

Choose a reason for hiding this comment

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

🚀 Just went through a code review and tested most of the affected flows

Copy link
Contributor

@jackkav jackkav left a comment

Choose a reason for hiding this comment

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

We had to break a lot of big rocks in one go in this PR to simplify an approach to keyboard shortcuts that was less indirect. Tested and working fine though.

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.

Request URL removed when request is duplicated (via keyboard shortcut)
5 participants