-
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
Add new block commands #52509
Add new block commands #52509
Conversation
Size Change: +1.33 kB (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
Flaky tests detected in 04f702a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5831950258
|
@@ -165,6 +165,7 @@ export { default as __experimentalInspectorPopoverHeader } from './inspector-pop | |||
|
|||
export { default as BlockEditorProvider } from './provider'; | |||
export { default as useSetting } from './use-setting'; | |||
export { useBlockCommands } from './use-block-commands'; |
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.
Do we need the Block
in the name of this hook. It's already part of block editor so maybe it can be just useCommands
which would stand for (use all block editor commands)
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.
I think it's fine to be granular? Is there any downside?
name: 'core/block-editor/blockActions', | ||
hook: useActionsCommands, | ||
} ); | ||
}; |
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.
Both of these command loaders use a lot of selectors... I do wonder about a potential performance impact on the command palette with long posts. Maybe it's fine though. One follow-up here could be to add a performance metric about the command palette.
607d005
to
2e0709d
Compare
@youknowriad do you think this can land as is? Should I try and improve anything in particular? I see the tests (including perf.) don't complain. |
Sure but let's get more testing first. |
2739e81
to
66b6600
Compare
@jameskoster I think you should see transforms and some of the basic block actions like move, insert before etc. I tested to see if the PR is having some issue and apparently it's all right couldn't repro. Maybe its your build? In terms of UI - when do we know to suggest what? |
Still only seeing the transform commands 🤔 Tried both post and site editors.
Worth a sanity check from @WordPress/gutenberg-design but I think it might be good to surface actions that apply to the selected block when the command palette is invoked. If you can detail exactly which commands this PR will add, we can work out priorities etc. |
@jameskoster this PR adds:
Showing all of them by default may be overwhelming. |
Agreed. Since the aim of this PR is to add so many commands, let's not surface any suggestions yet. We can explore that in a follow-up with a thought-out design which considers how to scale the display of so many commands, if necessary. |
66b6600
to
faf38ce
Compare
I've rebased and updated the PR with the comments from @richtabor . The icons part was a bit tricky as they are required by I made do with the icons we have. I don't think this to be a blocker as any visual tweaks can come in a follow up. I suggest we merge this to see if in the real world it causes any issues. Also I left the move to command on because it is extremely useful in distraction free mode, with the caveat that it looks bad :) |
packages/block-editor/package.json
Outdated
@@ -66,6 +67,7 @@ | |||
"@wordpress/wordcount": "file:../wordcount", | |||
"change-case": "^4.1.2", | |||
"classnames": "^2.3.1", | |||
"clipboard": "2.0.11", |
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.
This is a new dependency.
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.
In other places, we just use event.clipboardData.setData
…ames, use add instead of insert
This reverts commit e6d70b2.
e6d70b2
to
7a2ad93
Compare
I have reverted 6662c28 and removed copy and paste styles. The idea is to land the two hooks here with actions and transforms and see if there are any issues (perf) with them. For the copy and paste actions I'll follow up in another PR so we can have the time to spend on it without holding the rest back. |
Array.isArray( clientIds ) ? clientIds[ 0 ] : clientIds | ||
); | ||
return { | ||
possibleBlockTransformations: getBlockTransformItems( |
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.
In some contexts, this is triggering failures in the command loader. I'm guessing when the "blocks" array is empty.
cc @draganescu
Also @artpi for more details.
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.
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.
I am not sure what to do to trigger a visible error. Ultimately empty blocks
is guarded in getPossibleBlockTransformations
by:
if ( ! blocks.length ) {
return [];
}
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.
Yes, I see that in the code and yet I get the above stack trace. My suspicion is that we're getting [null, null ]
or something similar
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.
I finally have a reproduciton path. It took me quite a while:
What?
Advances: #51559
Adds block selection related commands: block transforms and block duplicate, copy, remove, insert after/before etc.
Why?
Mainly to improve the usefulness of the command center, but also because this improve the distraction free mode experience by offering immediate access to basic functions without turn off the mode.
How?
Register the commands using mostly copied functionality from the block switcher and block actions components.
Testing Instructions
Screenshots or screencast
block-commands.mp4
To do