-
Notifications
You must be signed in to change notification settings - Fork 26
Add Command Palette support #121
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
Conversation
Includes a refactor to divide commands between core and post-type based: core commands require admin permissions, whereas post-types are dynamic and depend on their own capabilities.
…similar to modules
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 left some initial general feedback before trying to better understand how it exactly works. I will try to test it next.
You can also divide the work into two PRs since it’s fully functional and tests well. Command Palette support doesn’t have to be a Beta Feature because it’s a straightforward enhancement that anyone familiar with the concept should appreciate. Follow-up work could include:
However, we need to take a closer look at the functionality, particularly the strategy for when these commands become available, depending on the WP version, the presence of custom post types, and user permissions. |
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
/** | ||
* Register admin commands for SCF | ||
*/ | ||
queue.add( context, () => { |
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 not really clear to me why you're using "priority-queue" here? Did you actually notice performance issues or something. Also potentially you can just use requestIdleCallback
directly instead of using a queue with a single callback.
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 did not notice any performance difference; I implemented the priority queue in parallel to defer the loading 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.
Let's just get rid of it. I think defer
is enough.
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 simplified to a requestIdleCallback
here before reading your answer. Should I get rid of that, too? It still seems overkill.
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 parallel to this PR, I wonder if we should implement this PR within SCF (before it gets into Core) to enable the command palette everywhere.
So for me, the PR is fine. But I think we should ask ourselves what we want to do with these post types and any SCF UI going forward. Do we want to decouple the server and client and have a consistent data layer to communicate between both (ideally on top of core-data) or do we want to continue with these localized scripts which for me feels like hacks. An investment in the core-data layer will help this PR but also any future PR that would like to rely on SCF entities in its client UI. |
I don't know if a hack, but it did feel like a dated approach that doesn't align with the current direction of core and modern architecture. I do agree with investing in decoupling server and client; the amount of data required in this PR is minimal, but as we move forward with new functionalities, it will pay off or even become necessary. How do you all feel about this PR with the current approach, and then follow up with the new endpoint implementation? |
I'm ok with this personally, but I would try to avoid the |
$plural_label = $post_type['labels']['name'] ?? $post_type['label'] ?? $post_type['post_type']; | ||
$singular_label = $post_type['labels']['singular_name'] ?? $post_type['singular_label'] ?? $plural_label; | ||
|
||
$post_type_obj = get_post_type_object( $post_type['post_type'] ); |
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 ask not knowing acf_get_acf_post_types
: is it ever possible to have a key returned by acf_get_acf_post_types
whose type object cannot be found? When?
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 theory it could in very edge cases, but we already checked empty( $post_type['post_type'] )
here, in which case this line wouldn't execute. The later checking of $post_type_obj
also ensures this line executes properly.
It's better in the long run to have "proper" communication, but — when we get to that — let's ensure that these payloads are preloaded, which is something we shouldn't take for granted in the old-school localize_script method. |
Thanks, I'll address the feedback and merge the PR; I just submitted the required changes to the |
Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>
1c1a584
to
857b84f
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.
We need to address the i18n issues right after this, but ship it 🚢
Thanks for the feedback, based on our last chat, the last commit directly uses the |
What
This PR adds support for WordPress Command Palette/Core Commands (Cmd+K / Ctrl+K interface) by registering:
Implementation details
The commands are divided into admin and CPT command files.
show_ui
settingTo prevent errors and optimize performance, JavaScript files for core and/or CPT commands will only be enqueued when needed.
Testing Instructions
editor
should only see CPT commandsRecording
Grabacion.de.pantalla.2025-04-25.a.las.13.55.21.mov
Followups
useCommandLoader
instead, which was introduced in WP 6.4.acf_localize_data