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 youknowriad commented Dec 24, 2019

Testing instructions

  • Check that undo, redo and save shortcuts work properly.

@youknowriad youknowriad added [Type] Code Quality Issues or PRs that relate to code quality [a11y] Keyboard & Focus labels Dec 24, 2019
@youknowriad youknowriad self-assigned this Dec 24, 2019
@youknowriad youknowriad merged commit 3757147 into master Dec 25, 2019
@youknowriad youknowriad deleted the update/visual-keyboard-shortcuts branch December 25, 2019 07:05
import { __ } from '@wordpress/i18n';
import { BlockEditorKeyboardShortcuts } from '@wordpress/block-editor';

function EditorKeyboardShortcutsRegister() {
Copy link
Member

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,
Copy link
Member

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?

Copy link
Contributor Author

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.

Comment on lines 107 to 110
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.
Copy link
Member

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();
Copy link
Member

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.

@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
@priethor priethor added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants