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

KeyboardShortcuts: Use a separate Mousetrap instance per component instance #2169

Merged

Conversation

jblz
Copy link
Contributor

@jblz jblz commented Aug 2, 2017

Fixes the same issue as #2085, but allows for multiple bindings per key command (props @aduth for the alternate approach).

Without this change, the callbacks are not being properly unbound when the visual editor unmounts.

To Test

Confirm Existing Behavior

  • Run release or master
  • Create a new post via: /wp-admin/admin.php?page=gutenberg
  • Add some content
  • Switch to text mode
  • Issue a keyboard command...for example: cmd+a for "select all"
    NOTE: You may need to click the editor whitespace again to be able to kick this off
  • Confirm the Visual Editor's select all callback is executed:

screen shot 2017-07-29 at 1 29 05 pm

Confirm the fix

  • Run this branch
  • Repeat the above
    • Callbacks should only be executed in the visual editor mode (none are currently registered via this component in text mode)
  • Attempt to bind the same key command(s) as are already bound via a separately instantiated KeyboardCommands compoent
    • All bound callbacks should execute and unbind when the component unmounts.

…stance

Fixes the same issue as WordPress#2085, but allows for multiple bindings per key command.
@codecov
Copy link

codecov bot commented Aug 2, 2017

Codecov Report

Merging #2169 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2169   +/-   ##
=======================================
  Coverage   22.23%   22.23%           
=======================================
  Files         136      136           
  Lines        4273     4273           
  Branches      721      720    -1     
=======================================
  Hits          950      950           
- Misses       2806     2807    +1     
+ Partials      517      516    -1
Impacted Files Coverage Δ
components/keyboard-shortcuts/index.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c390037...1ff72df. Read the comment docs.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I don't know what's going on with the codecov failure, but this looks good to me 👍

@aduth
Copy link
Member

aduth commented Aug 2, 2017

Aside: I think this will make for an easier path toward context-scoped keyboard bindings, as mentioned in "Implementation notes" of #1944

@aduth aduth merged commit 1954f50 into WordPress:master Aug 2, 2017
@jblz jblz deleted the add/mousetrap-instance-per-keyboard-shortcuts branch August 3, 2017 03:57
@jblz jblz mentioned this pull request Aug 3, 2017
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