-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Converts paragraphs to headings with keyboard shortcuts #44681
Conversation
Size Change: +1.05 kB (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
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.
If these shortcuts are specific to the block then it seems reasonable to register them on the block itself.
I wonder:
- will this register the shortcuts repeatedly for each instance of the block?
- do you need to have a method of _de_registering the shortcuts if the block is not in use (see this example)?
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.
It's a nice convenience feature. 👍
The thing that stood out in testing is there's no easy way to reverse the action (other than undo). Maybe a modifier+0
could be an option for transforming back to a paragraph.
Also, once you've converted to a heading it should be possible to continue using the shortcut to switch between other heading levels, so I think the same shortcuts could be added to the heading block.
Adding this code directly to the blocks is a reasonable first iteration. I think there might be other options that are worth exploring.
The shortcuts are really just block transforms, so perhaps it should be possible to specify a keyboard shortcut for a transform and have the shortcut display in the transform menu:
I feel like that would be an interesting solution, but not without problems - at the moment the only transform between a paragraph and heading is for the default Heading block which is always a <h2>
level. There's some overlap here with #32472 and #33811, which suggests showing each heading level in the quick inserter and there was a previous attempt to add the levels as variations, but seems it was abandoned. There's lots of opportunities here, but it's just a matter of finding the right solution.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
4fe4bd3
to
a4a4d99
Compare
Converted this to draft as it is currently breaking other rich text keyboard interactions. |
I think this should be refactored in a way in which it is the post editor's job to handle these shortcuts. I'll refactor to try and replicate in the post editor how the block editor handles block level shortcuts. |
a4a4d99
to
f9db6de
Compare
- Moves the functionality from the paragraph block to the block editor package - Implements the shortcuts for paragraph and heading allowing cycling between them - Refactors the code in a less repetitive shape - Adds access+0 as a way to convert headings to paragraphs - Implements cycling through heading levels via access+[1-6] - Registers shortcuts only once - Adds a new prop to RichText allowing for onKeyDown custom handlers - Removes the keyboard-shortcuts dependency from block libary Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Dave Smith <[email protected]>
fded516
to
dc19a20
Compare
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.
Works really well now. I think a few aspects of the code could be tidied up, but it's not far off being ready. Nice work!
packages/edit-post/src/components/keyboard-shortcut-help-modal/config.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/keyboard-shortcut-help-modal/config.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/keyboard-shortcut-help-modal/config.js
Show resolved
Hide resolved
… and handler into edit post shortcut conf file. Co-authored-by: Daniel Richards <[email protected]>
…or multiselection and avoiding an extra state select. Co-authored-by: Daniel Richards <[email protected]>
Cool review @talldan 👏🏻 Applied all feedback, ready for a new check. |
So I tested this as fan of keyboard shortcut so yah for @draganescu to tackle. Nice, it already listed in the Keyboard shortcuts page! Love it. :-) |
class="edit-post-keyboard-shortcut-help-modal__shortcut-term" | ||
> | ||
<kbd | ||
aria-label="Shift + Alt + 0" |
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.
Isn't it Ctrl + Alt + 0?
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.
Hmm maybe that is not access?
<kbd | ||
class="edit-post-keyboard-shortcut-help-modal__shortcut-key" | ||
> | ||
Shift |
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.
Ctrl?
class="edit-post-keyboard-shortcut-help-modal__shortcut-term" | ||
> | ||
<kbd | ||
aria-label="Shift + Alt + 1 6" |
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.
Ctrl?
<kbd | ||
class="edit-post-keyboard-shortcut-help-modal__shortcut-key" | ||
> | ||
Shift |
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.
Ctrl?
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.
access: ( _isApple ) => ( _isApple() ? [ CTRL, ALT ] : [ SHIFT, ALT ] ),
The tests run on a linux env so the snapshots match :)
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.
LGTM.
✅ tested converting paragraphs to heading 1, 2, 3, 4, 5, 6
✅ tested converting headings to a paragraph
@draganescu |
@t-hamano sure thing, I think it's a cool addition from a consistency point of view. The intended goal here is to improve the writing experience via easier formatting - in the widget and site editors we expect more of a structural approach and less focus on flow, but sure, even cycling through heading sizes with a KBD shortcut is awesome. Add me to review! 🙇🏻 |
What?
Closes #11173
Why?
For feature parity with the classic editor in WordPress but also because they're a great set of shortcuts to have.
How?
Not sure I am doing it right, because:
switchToBlockType
receive someattributes ...)?
Testing Instructions
Screenshots
document-level-cycle.mp4
To do
The mechanism in the heading block would be different as it's only a "level" change. So maybe a separate PR?
access+[1-6]
is for pinned items