-
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
Enable the command palette feature everywhere in WordPress #54515
base: trunk
Are you sure you want to change the base?
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @jonathanbardo! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
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 like this proposal personally. I left some technical comments.
That said, I believe we need a product decision here first. Would love more input from @mtias @jasmussen @richtabor @SaxonF
|
||
useShortcut( |
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 should still use the useShortcut for several reasons: cross browser, cross platform and the ability to edit the shortcode programmatically.
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.
@youknowriad I pulled the latest trunk changes and now the hooks works as expected. So I reverted the file in 0096521
packages/core-commands/src/index.js
Outdated
); | ||
} | ||
|
||
root.render( <CommandMenuWrapper /> ); |
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.
While I like this way of rendering the command menu everywhere, I'm not sure that the "core-commands" package whose sole purpose is to have a list of commands that works cross pages. Would a small inline script work instead of a package for this?
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'm having second thoughts here, so I'd love more opinions here @WordPress/gutenberg-core
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 agree it doesn't seem the right approach but there was a circular dependency if I implemented this inside of the command component directly. So perhaps it needs to be it's own component loader completely.
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'll try the approach of the inline script. Good idea :)
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.
Inline script done in 2873f18
packages/edit-post/src/editor.js
Outdated
@@ -13,7 +13,7 @@ import { useMemo } from '@wordpress/element'; | |||
import { SlotFillProvider } from '@wordpress/components'; | |||
import { store as coreStore } from '@wordpress/core-data'; | |||
import { store as preferencesStore } from '@wordpress/preferences'; | |||
import { CommandMenu } from '@wordpress/commands'; | |||
// eslint-disable-next-line no-unused-vars | |||
import { privateApis as coreCommandsPrivateApis } from '@wordpress/core-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.
If it's not used, just remove it no?
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.
@youknowriad yes I tried that but it didn't work. It needs the import to work or the commands do not get loaded at all. Not sure how to explain it still.
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.
Removed in 2873f18
wp_enqueue_script( 'wp-core-commands' ); | ||
} | ||
|
||
add_action( 'wp_print_scripts', 'gutenberg_enqueue_commands', 1 ); |
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 should probably move this to lib/wordpress-6.4
(or 6.5) depending on when the PR lands.
Personally, I'd like to see the logged accessibility issues fixed before this goes anywhere near Core-wide. There are already some PRs in progress to fix things up but reviews/further assistance would be helpful. Thanks. |
Congratulations on your first PR, @jonathanbardo! This change makes a lot of sense to me in the scope of Phase 3 and the admin redesign. Given that we are only a few days from WordPress 6.4 Beta1, this could be targeted towards 6.5 so that there's enough time to test it and work on the a11y issues mentioned above. |
Exactly what I was thinking. Love the potential here. |
I tested this PR again. In some cases, it may cause unintended behavior.
|
@t-hamano I fixed the issue regarding the commands not loading in the widget editor. Thanks for pointing this out. I'm still trying to wrap my head around how Gutenberg enqueues and overrides scripts and styles. The post editor should work too. One thing I've noticed at the moment is that the |
packages/core-commands/src/index.js
Outdated
@@ -1 +1,2 @@ | |||
export { unlock } from './lock-unlock'; |
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.
Unfortunately this is not something we should export but it makes "private APIs" public for any third-party plugin and this shouldn't be the case.
Maybe we should just export the CommandWrapper component like we used to do? 🤔
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 exported a commands-menu-wrapper component from the core-commands packages and used a small inline script to register it on the page in d381a0d. I honestly would have preferred to this wrapper in the commands package instead but the circular dependency doesn't make this simple.
Add small inline script to load CommandsMenuWrapper
Cool PR! Let's aim to add this behind a flag in the experiments page until we are comfortable with wider rollout. |
Pulling in feedback from a Core Editor Improvement post comment that's in favor of this PR:
Going to respond to them with a link to this work but wanted to note it as we consider priorities. |
There are definitely plugins that we maintain (https://github.com/10up) that would make use of this outside the editor, though we'd want/need the ability to only have certain commands exist within the context of the editor vs. elsewhere. So some context of where the palette is being triggered so our various plugins would properly integrate or not depending on that context. |
What?
Closed #58218.
This PR introduce the ability to have the command palette everywhere in WordPress and not just inside of the new editor experience.
Why?
After talking to a few people who got excited about the command palette at WordCamp US we realized that in order to increase adoption of the command palette it needed to be available everywhere (so that it becomes an habit to reach out for it much like Spotlight Search on mac).
How?
This PR removes the need to add the component in both editor packages (edit-site and edit-post) and instead registers it at the global level. When the script is on the page, then the command palette is there as well. Yeah editor are then able to register custom commands as required.
Some global commands might need to be reworked to be available in a global context. Some of them at the moment do not redirect to a valid url. This PR is mainly to open a conversation. Feedback welcomed :)
Testing Instructions
Testing Instructions for Keyboard
Press the command palette keystroke combination to invoke the global component.
Screenshots or screencast