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

Add a new keyboard shortcut package #19100

Merged
merged 26 commits into from Dec 23, 2019
Merged

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Dec 12, 2019

There's a few ideas in this PR:

  • Create a new package that handles the keyboard shortcuts in a data store:
    • Ultimately this allows us to implement a UI to change the keyboard combination per user.
    • Potentially, this also allows third-party shortcuts and shortcut categories.
  • Create a useShortcut hook as a replacement to the KeyboardShorcuts package but there's no need to pass the actual combination since it's retrieved from the store.
  • Refactors the BlockEditorKeyboardShortcuts component to use this new package. (Ultimately, all shortcuts should rely on this package).
  • While refactoring BlockEditorKeyboardShortcuts, I moved some logic we had in components into data actions (DuplicateBlocks, InsertBefore, InsertAfter actions) and also handle the checks in the action itself. Removing selectors calls from components into one of actions can have potential performance boost and also allow easy logic sharing. Related here, I think the BlockActions component should be deprecated and removed in favor of selectors and actions.

Once all the keyboard shortcuts refactored to use this package, the whole modal could be generated without any hard-coded code.

@youknowriad youknowriad requested review from ellatrix and talldan as code owners Dec 12, 2019
@youknowriad youknowriad self-assigned this Dec 12, 2019
@youknowriad youknowriad requested review from aduth, afercia and WordPress/gutenberg-core Dec 12, 2019
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 12, 2019

It's a huge PR and I won't have time to review but I applaud the efforts. Great idea 💯

Copy link
Contributor

MarcoZehe left a comment

I found some misspellings of the word "shortcut" throughout the files and suggested corrections so they don't look weird.

@youknowriad youknowriad force-pushed the add/keyboard-shortcuts-package branch 2 times, most recently from d810e97 to 6708224 Dec 17, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 17, 2019

I updated the Modal accordingly.

packages/keyboard-shortcuts/src/store/selectors.js Outdated Show resolved Hide resolved
packages/keyboard-shortcuts/src/store/actions.js Outdated Show resolved Hide resolved
packages/keyboard-shortcuts/src/store/actions.js Outdated Show resolved Hide resolved
packages/keyboard-shortcuts/src/hooks/use-shortcut.js Outdated Show resolved Hide resolved
packages/keyboard-shortcuts/src/hooks/use-shortcut.js Outdated Show resolved Hide resolved

shortcutKeys.forEach( ( shortcut ) => {
const keys = shortcut.split( '+' );
const modifiers = new Set( keys.filter( ( value ) => value.length > 1 ) );

This comment has been minimized.

Copy link
@aduth

aduth Dec 17, 2019

Member

What's the single-character modifier we're filtering here? Comment or externalized/named function could help clarify.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 18, 2019

Author Contributor

I can't tell. This is ported from KeyboardShortcuts.

This comment has been minimized.

Copy link
@talldan

talldan Dec 20, 2019

Contributor

It looks like it determines whether a key is a modifier by the length of the string. E.g. if I add a pass a shortcut Shift+Cmd+M, it'll determine that the modifiers are Shift and Cmd because they're not a single character.

That does seem a little bit unusual. Might be safer to check against an array of known modifiers.

This comment has been minimized.

Copy link
@aduth

aduth Dec 20, 2019

Member

It looks like it determines whether a key is a modifier by the length of the string. E.g. if I add a pass a shortcut Shift+Cmd+M, it'll determine that the modifiers are Shift and Cmd because they're not a single character.

That sounds about right.

Might be safer to check against an array of known modifiers.

I'd have been content if the original implementation had simply added an inline comment, to save ourselves the struggle in trying to decipher its meaning 😅

Copy link
Contributor

talldan left a comment

I really like the direction this is taking. 👍

I spotted a couple of bugs to be fixed - shortcuts not working and the unregister action has the wrong type.

packages/block-editor/src/store/actions.js Outdated Show resolved Hide resolved
packages/block-editor/src/store/actions.js Outdated Show resolved Hide resolved
packages/keyboard-shortcuts/src/store/actions.js Outdated Show resolved Hide resolved
packages/keyboard-shortcuts/src/store/actions.js Outdated Show resolved Hide resolved
// Registering the shortcuts
const { registerShortcut } = useDispatch( 'core/keyboard-shortcuts' );
useEffect( () => {
registerShortcut(

This comment has been minimized.

Copy link
@talldan

talldan Dec 18, 2019

Contributor

In terms of displaying a list of shortcuts in the help modal, it seems great that this can be done dynamically using the values from the store, and it might be good to move towards this more.

The main issue I see with shortcuts being registered in this way is that it could result in a situation where a component with shortcuts hasn't yet been rendered, meaning the shortcut isn't registered and so the description won't appear in the help modal.

I wondered if an api more like registerBlock in the blocks api might be preferable to get around that issue and then have these registered outside of the component scope.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 18, 2019

Author Contributor

I think registration belongs to the initialization of a package. In this case, the block-editor package does both at the same time but I agree that we should be careful about that.

I do think it should always be tied to a component though and registerBlock is the wrong approach here (can't change now) to avoid globals. See #8981 for the problems of the globals approach.

This comment has been minimized.

Copy link
@talldan

talldan Dec 19, 2019

Contributor

I might be wrong, but I think a similar issue exists today with the KeyboardShortcuts component anyway. If I remember correctly that's why there was a separate BlockActions component in the first place—the shortcuts have to be rendered in a different part of the render tree. So it might be a moot point, if the component isn't rendered the shortcuts won't work.

There's one case where this is still a little problematic, which is if you load the editor in Code Editor mode and then open the help modal, Code Editor elminates a huge part of the render tree, including the shortcut registration. Most of the Block shortcuts are then not visible. 😄

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 19, 2019

Author Contributor

There's one case where this is still a little problematic, which is if you load the editor in Code Editor mode and then open the help modal, Code Editor elminates a huge part of the render tree, including the shortcut registration. Most of the Block shortcuts are then not visible.

That's a good point, I'll fix.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 19, 2019

Author Contributor

I split the registration and the handling into two components.

@youknowriad youknowriad requested review from talldan and aduth Dec 18, 2019
Copy link
Contributor

talldan left a comment

@youknowriad I took if for a one last spin and it's all working well. Code looks good to me too. 🎉

Only thing that might be nice is tests for those new selectors and reducers, but the bulk of the code looks great and I'm about to go away until 2020. 😄

youknowriad and others added 2 commits Dec 12, 2019
Co-Authored-By: Daniel Richards <daniel.richards@automattic.com>
@youknowriad youknowriad force-pushed the add/keyboard-shortcuts-package branch from c450eb9 to 6aa4d62 Dec 20, 2019
keyCombination: '/',
description: __( 'Change the block type after adding a new paragraph.' ),
/* translators: The forward-slash character. e.g. '/'. */
ariaLabel: __( 'Forward-slash' ),

This comment has been minimized.

Copy link
@aduth

aduth Dec 20, 2019

Member

Should this be an option in the shortcut registration? I see there's a few more usages of this in globalShortcuts as well.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 20, 2019

Author Contributor

Potentially yes, we'll probably find a few other options once we refactor all existing shortcuts.

packages/keyboard-shortcuts/src/store/actions.js Outdated Show resolved Hide resolved
packages/keyboard-shortcuts/src/store/actions.js Outdated Show resolved Hide resolved
packages/keyboard-shortcuts/src/store/selectors.js Outdated Show resolved Hide resolved

shortcutKeys.forEach( ( shortcut ) => {
const keys = shortcut.split( '+' );
const modifiers = new Set( keys.filter( ( value ) => value.length > 1 ) );

This comment has been minimized.

Copy link
@aduth

aduth Dec 20, 2019

Member

It looks like it determines whether a key is a modifier by the length of the string. E.g. if I add a pass a shortcut Shift+Cmd+M, it'll determine that the modifiers are Shift and Cmd because they're not a single character.

That sounds about right.

Might be safer to check against an array of known modifiers.

I'd have been content if the original implementation had simply added an inline comment, to save ourselves the struggle in trying to decipher its meaning 😅

youknowriad and others added 6 commits Dec 20, 2019
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
@youknowriad youknowriad merged commit fba1069 into master Dec 23, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the add/keyboard-shortcuts-package branch Dec 23, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.