Abstract keyboard shorcuts for heading to paragraph transform and vice-versa #60606
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -692 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
afercia
force-pushed
the
fix/abstract-heading-paragraph-transform-keyboard-shorcuts
branch
from
f957fdb
to
adc45ec
Compare
|
Flaky tests detected in 678938f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8686828386
|
afercia
requested review from
draganescu,
talldan,
adamziel,
kevin940726 and
ellatrix
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Thanks for opening the PR, indeed it seems that there's some duplication here. In some cases duplication is better than abstraction if we can't think of the right abstraction at a given time. That said, for this one, It seems that the best place for the shared code for these is in the "block-library" package IMO because this is block specific code (some block-editors might not have paragraph or heading blocks or might not use the block library at all) |
|
@youknowriad thanks. So basically, follow the approach of the first commit with a new |
|
@afercia yes, that would be a good first approach. The downside would be the need to "import" this component in each implementation. I kind of wish that registering the blocks should be enough for these shortcuts to work. Don't we have transform "types" that handle these for us already (cc @ellatrix) ? If it's not the case, maybe fine to consider a separate issue for that. |
|
Yes, Ideally we reuse the block transforms API for this. Maybe add a shortcut prop to those objects? |
in #60567 I would like to extend these keyboard shortctus to work also with other blocks e.g. the Site Title. These blocks don't use a 'transform'. Rather, the HTML tag they are rendered with is controlled by the block attribute |
afercia
force-pushed
the
fix/abstract-heading-paragraph-transform-keyboard-shorcuts
branch
from
adc45ec
to
e80b714
Compare
|
Last commits moves shared code to the 'block-library' package. I had to add |
youknowriad
reviewed
Apr 12, 2024
youknowriad
reviewed
Apr 15, 2024
| import { createBlock } from '@wordpress/blocks'; | ||
| import { store as blockEditorStore } from '@wordpress/block-editor'; | ||
|
|
||
| function BlockKeyboardShortcuts() { |
There was a problem hiding this comment.
In this component registration and usage is merged. They were in different components before. The reason is that in some contexts, we want to register the shortcuts (show them in the keyboard shortcuts modal) but "disable" them in some contexts (view mode in the site editor for instance).
I'm not sure whether this creates issues for this PR, it needs to be tested.
There was a problem hiding this comment.
It seems the Block prefix is redundant here as well, we're already in the block library package.
There was a problem hiding this comment.
I can rename it but then I'd have to import it 'as' a different name you know, because other components are already named KeyboardShortcuts.
There was a problem hiding this comment.
yeah, we do that already for other things. I don't care much though, it's a private API anyway.
There was a problem hiding this comment.
In this component registration and usage is merged. They were in different components before.
I think they were separated only in edit-site, where the use was here? Not sure that makes a difference, because they are within a check for isEditMode?
There was a problem hiding this comment.
yes, for this PR it probably doesn't change much.
youknowriad
reviewed
Apr 15, 2024
afercia
force-pushed
the
fix/abstract-heading-paragraph-transform-keyboard-shorcuts
branch
from
7b9bf8f
to
678938f
Compare
youknowriad
approved these changes
Apr 22, 2024
afercia
force-pushed
the
fix/abstract-heading-paragraph-transform-keyboard-shorcuts
branch
from
678938f
to
ce27aeb
Compare
|
Rebased on top of latest trunk, hopefully tests will pass. |
afercia
deleted the
fix/abstract-heading-paragraph-transform-keyboard-shorcuts
branch
bph
added
the
[Focus] Accessibility (a11y)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #60567
What?
As a preparatory work for #60567, this pull requests seeks to clean up and avoid code duplication for the keyboard shortcuts that are responsible for transforming heading levels and paragraph to heading and vice-versa.
Currently, the code responsible for this feature is duplicated in 4 different locations in the codebase: post editor, sit editor, widgets editor and customize widgets.
I'd appreciate some feedback about the 'architectural' choice of where to move this code. See the 'How' below.
Cc @youknowriad @WordPress/gutenberg-core when you have a chance 🙏🏽
Additionally, this PR adds a new keyboard shortcut alias
7to transform a Heading to Paragraph, for consistency with the Classic Editor where this shortcut has always used the number 7.Why?
Code duplication brings in maintenance cost and potential bugs and inconsistencies. Havin a centralized, unique, code responsible for a feature allow to easily change it, extend it, improve it, test it.
How?
BlockKeyboardShortcutscomponent and add that to the stack. This was maybe cleaner but it added a new component.KeyboardShortcutscomponent and their 'callbacks' use a custom implementation in theBlockToolscomponent. In the last commit I moved everything to follow this pattern. However, I'm not sure this existing implementation is ideal, as it mixes a component that does many other things (maybe too many) with the keyboard shortcuts.Any suggestion on which of the two approaches would be better is welcome.
Testing Instructions
npm installin your terminal before building, as the 'block-library' package has now a new dependency.Convert the current heading to a paragraph.now displays also the new alias with thte key7.Testing Instructions for Keyboard
Screenshots or screencast