-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
import { __ } from '@wordpress/i18n'; | ||
import { BlockEditorKeyboardShortcuts } from '@wordpress/block-editor'; | ||
|
||
function EditorKeyboardShortcutsRegister() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this should have been unindented as well.
shortcutKeys.forEach( ( shortcut ) => { | ||
mousetrap.unbind( shortcut, eventName ); | ||
} ); | ||
mousetrap.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍 Didn't know this existed.
Testing instructions