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

Refactor the visual editor shortcuts to use the keyboard-shortcuts package #19318

Merged
merged 6 commits into from Dec 25, 2019

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Dec 24, 2019

Testing instructions

  • Check that undo, redo and save shortcuts work properly.
youknowriad added 5 commits Dec 24, 2019
@youknowriad youknowriad merged commit 3757147 into master Dec 25, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the update/visual-keyboard-shortcuts branch Dec 25, 2019
import { __ } from '@wordpress/i18n';
import { BlockEditorKeyboardShortcuts } from '@wordpress/block-editor';

function EditorKeyboardShortcutsRegister() {

This comment has been minimized.

Copy link
@aduth

aduth Jan 2, 2020

Member

Nit: There's inconsistency here in that the file is named as "register shortcuts" but the component as "shortcuts register".

@@ -12,11 +12,11 @@ import {
UnsavedChangesWarning,
EditorNotices,
PostPublishPanel,
EditorKeyboardShortcutsRegister,

This comment has been minimized.

Copy link
@aduth

aduth Jan 2, 2020

Member

Why do we have inconsistency that from the block-editor package, the component which manages shortcut registration is defined as BlockEditorKeyboardShortcuts.Register (a property of BlockEditorKeyboardShortcuts), but then the same sort of component from editor package has its own standalone exported member EditorKeyboardShortcutsRegister?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 3, 2020

Author Contributor

because the Editor one registers both the Text and Visual shortcuts that can't be applied at the same time, another option is to split the registration into two components as well but I feel a better pattern is that each package would have a single register component.

Only autofocus the title when the post is entirely empty.
This should only happen for a new post, which means we
focus the title on new post so the author can start typing
right away, without needing to click anything.
Comment on lines 107 to 110

This comment has been minimized.

Copy link
@aduth

aduth Jan 2, 2020

Member

Seems this should have been unindented as well.

shortcutKeys.forEach( ( shortcut ) => {
mousetrap.unbind( shortcut, eventName );
} );
mousetrap.reset();
Comment on lines -81 to +81

This comment has been minimized.

Copy link
@aduth

aduth Jan 2, 2020

Member

Nice 👍 Didn't know this existed.

@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
Projects
None yet
2 participants
You can’t perform that action at this time.