-
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
Add block-specific commands as contextual suggestions [#53539] #53974
Conversation
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.
Thanks for the PR @jrtashjian! I'll cc @WordPress/gutenberg-design for any input about the UX and the prominence of these commands.
@@ -124,7 +131,9 @@ function useEditorStyles() { | |||
} | |||
|
|||
function Layout() { | |||
useCommands(); |
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.
We already have the hook in edit post here.
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 can remove it from the Editor component and leave it where I initially added it in the Layout component. This will make sure things stay consistent between edit-post and edit-site packages.
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.
Done in 5e96629.
I think it looks good. The pencil icon for the Remove action threw me though. Should that use the trash icon instead? |
@jameskoster this PR only exposes the existing commands in a contextual matter. I know these block level actions were recently merged into trunk though. I can update this branch with the latest on trunk to see if icons have been tweaked. |
Looks like no changes to the icons on trunk but I did run across this follow up issue for tweaking the icons: #53538 |
I've just pushed another PR up for those icon fixes mentioned in the issue I found: #54427 |
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.
Good idea with the special quick actions loader, it's the only way to filter out what we don't want to show in quick actions. Otherwise the code looks good.
It also tested well for me.
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
@jrtashjian mind rebasing, running the checks again? |
I rebased but there still seems to be a failing test. It's definitely unrelated to the command palette though. I'm not super familiar with the test suite yet but if someone else is able to help resolve it, I'd appreciate it. |
@jrtashjian do you mind rebasing/seeing if that fixes the test? 🙇♂️ |
@richtabor ok, rebased again and it looks like the previously failing tests passed. However the Performance Tests are taking quite a while to run. |
What?
This PR adds a new context named
block-selection-edit
which is applied to the command palette when a block (or group of blocks) are selected. This allows us to show the block actions in the command palette immediately rather than searching for them.Why?
Related to #53539, closes #53539.
How?
By adding additional logic to the edit-site and edit-post packages (when context is set) we can apply our new context. I could use assistance improving this to simplify the logic maybe with global context conditions that can be defined in one place rather than duplicating code.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast