Add keyboard shortcuts for moving blocks #23276
Conversation
ntsekouras
added
[a11y] Keyboard & Focus
[Type] Enhancement
|
Size Change: +112 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
ntsekouras
force-pushed
the
add/keyboard-support-for-moving-blocks
branch
from
5da21af
to
b210e44
Compare
ntsekouras
requested review from
ajitbohra,
ellatrix,
gziolo,
nerrad,
ntwb,
talldan and
youknowriad
as code owners
|
This is a really cool PR. I agree that we need to find better key combinations. cc @mcsf maybe. I got this on safari because of the current shortcuts
|
|
Thank you @youknowriad! I will change them for now to something that works for all ( Mac, Linux, Windows ), just for testing purposes. I will search first and then make a proposal for key combination, but everyone is welcome to share their thoughts. |
| * @param {string} start Identifier of the start block. | ||
| * @param {string} end Identifier of the end block. | ||
| */ | ||
| export async function multiSelectBlocks( start, end ) { |
There was a problem hiding this comment.
Good idea 👍
Do you think multiSelectBlocks could be further abstracted, at the moment it’s only a proxy call to data layer and there is a helper for that already:
It would be nice if someone using this new public API wouldn’t have to get ids but could pass something simpler like number of blocks to select.
There was a problem hiding this comment.
There is still the need for having a starting reference. For example select from selectMulti( 2, 5 ) that would select from the second block. I'll look into it! Nice idea @gziolo. I should also improve it by selecting the start block here.
ntsekouras
force-pushed
the
add/keyboard-support-for-moving-blocks
branch
from
3db4999
to
8328582
Compare
Gutenberg already has shortcuts for "Insert a new block before the selected block(s)." (⌥+⌘+T) and "Insert a new block after the selected block(s)." (⌥+⌘+Y). Could we reuse this pattern of T representing "above" and Y "below"? Maybe be adding a Shift to the shortcuts? |
packages/e2e-test-utils/README.md
Outdated
|
|
||
| _Usage_ | ||
|
|
||
| // selects three blocks -- from the second block to fourth (2, 3, 4). |
There was a problem hiding this comment.
Something is wrong with the example in JSDoc.
There was a problem hiding this comment.
Thanks! Fixed it.
@mcsf I believe this is great idea, as it would be intuitive to the user. I implemented this for test drive :) |
ntsekouras
force-pushed
the
add/keyboard-support-for-moving-blocks
branch
2 times, most recently
from
15d0be0
to
7717169
Compare
| getBlockRootClientId, | ||
| } = select( 'core/block-editor' ); | ||
| const selectedClientIds = getSelectedBlockClientIds(); | ||
| const normalizedClientIds = castArray( selectedClientIds ); |
There was a problem hiding this comment.
Looking at getSelectedBlockClientIds, it always returns an array. Any reason to array-cast into a new variable?
There was a problem hiding this comment.
You're right! Thanks! I removed it.
packages/block-editor/src/components/keyboard-shortcuts/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/keyboard-shortcuts/index.js
Outdated
Show resolved
Hide resolved
| <td>Move the selected block(s) up.</td> | ||
| <td><kbd>Ctrl</kbd>+<kbd>Alt</kbd>+<kbd>Shift</kbd>+<kbd>T</kbd></td> | ||
| <td><kbd>⌥</kbd><kbd>⌘</kbd><kbd>⇧</kbd><kbd>T</kbd></td> | ||
| </tr> | ||
| <tr> | ||
| <td>Move the selected block(s) down.</td> |
There was a problem hiding this comment.
See below suggestions for revised labels.
packages/e2e-test-utils/src/index.js
Outdated
| @@ -35,6 +35,10 @@ export { installPlugin } from './install-plugin'; | |||
| export { isCurrentURL } from './is-current-url'; | |||
| export { isInDefaultBlock } from './is-in-default-block'; | |||
| export { loginUser } from './login-user'; | |||
| export { | |||
| multiSelectBlocksByIds, | |||
There was a problem hiding this comment.
Could we consolidate those two methods into one API? Those methods are exposed as public API and added to the documentation. We will have to maintain them for long because the 3rd party project will use it.
There was a problem hiding this comment.
I was talking with @mcsf about making the e2e tests more user interactive and not multi select programmatically, so I'll remove them as they will not be used. Do you think it has any value to keep them for now, if not used?
There was a problem hiding this comment.
Not at all, we shouldn't keep methods that aren't used (unless they were part of public API previously).
| * multiSelectBlocksByRange( 2, 3 ); | ||
| * ``` | ||
| * // selects three blocks -- from the second block to fourth (2, 3, 4). |
There was a problem hiding this comment.
| * multiSelectBlocksByRange( 2, 3 ); | |
| * ``` | |
| * // selects three blocks -- from the second block to fourth (2, 3, 4). | |
| * multiSelectBlocksByRange( 2, 4 ); | |
| * // selects blocks at index 2, 3, 4 |
How about we change the API to passing startIndex and endIndex to remove the need to use code comments to explain how it works?
| */ | ||
| export function arePrePublishChecksEnabled() { | ||
| export async function arePrePublishChecksEnabled() { |
| */ | ||
| export function wpDataSelect( store, selector, ...parameters ) { | ||
| export async function wpDataSelect( store, selector, ...parameters ) { |
There was a problem hiding this comment.
Great catch, page.evaluate that is used is async:
It might make some tests more stable. /cc @youknowriad
There was a problem hiding this comment.
I don't think it makes any difference to add "async" here since page.evalute returns a promise already
There was a problem hiding this comment.
It doesn't make any difference but it's explicit that returns a Promise. So someone would know they have to await.
There was a problem hiding this comment.
In any case, the function signature was wrong, so this is a good catch.
ntsekouras
force-pushed
the
add/keyboard-support-for-moving-blocks
branch
from
dde125e
to
5dfe5c2
Compare
| it( 'should move the block up', async () => { | ||
| await createTestParagraphBlocks(); | ||
| expect( await getEditedPostContent() ).toMatchSnapshot(); | ||
| await Promise.all( [ moveUp(), moveUp() ] ); // press twice |
There was a problem hiding this comment.
Since we want testing to reflect real usage, we can't have parallel effects, so we need to split the effects out in sequence:
await moveUp();
await moveUp();There was a problem hiding this comment.
Thanks! Good note!
| const multiSelectBlocks = async () => { | ||
| await page.keyboard.down( 'Shift' ); | ||
| await page.keyboard.press( 'ArrowUp' ); | ||
| await page.keyboard.up( 'Shift' ); | ||
| }; |
There was a problem hiding this comment.
moveUp and moveDown are unequivocal in their effects, and there is only one possible means to trigger each of them, so they can make sense as functions.
In contrast, multiSelectBlocks isn't unique: there are many ways to cause multiple blocks to be selected, and so this function depends on specific motions (in this case, upward). This means that when a particular test calls multiSelectBlocks, it's not clear for the reader what is happening in terms of editor state, unless they go up to read the definition of multiSelectBlocks.
This isn't a hard "no", but it's my own interpretation of what can improve the readability of tests.
There was a problem hiding this comment.
You're not wrong about this. I changed it be more readable.
Mark functions that use wpDataSelect as async as they return Promise. It should be clear how to consume them.
Co-authored-by: Miguel Fonseca <[email protected]>
Co-authored-by: Miguel Fonseca <[email protected]>
ntsekouras
force-pushed
the
add/keyboard-support-for-moving-blocks
branch
from
a6653e0
to
e6d6960
Compare
|
Here is an associated PR which just got merged: |
As the title suggests, there's still some stuff to iron out around the "Move to …" feature. The two PRs aren't mutually exclusive, so let's merge this one. I see #22453 as more compelling as an accessibility improvement and as a QoL improvement when editing complex arrangements with nested blocks. Meanwhile, this PR falls more in the writing flow category, satisfying the need for quick motion around the selected block. |
|
was this abandoned? |
@m0rg5 this has been merged for a while now.. |
Description
Resolves #10455
This PR adds keyboard shortcuts for moving blocks up and down.
Keyboard combinations for macOS:
Windows/Linux:
How has this been tested?
Screenshots
Types of changes
Checklist: