Skip to content

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

Merged
merged 41 commits into from
May 14, 2025
Merged

Add Command Palette support #121

merged 41 commits into from
May 14, 2025

Conversation

priethor
Copy link
Contributor

@priethor priethor commented Apr 25, 2025

What

This PR adds support for WordPress Command Palette/Core Commands (Cmd+K / Ctrl+K interface) by registering:

  • Core commands to navigate to SCF admin pages
  • "View All" commands for each Custom Post Type registered with SCF
  • "Add new" for each Custom Post Type registered with SCF

Implementation details

The commands are divided into admin and CPT command files.

  • Core commands are only registered when the current user has admin permissions
  • CPT commands are registered only when these conditions are met:
    • The current user has enough capabilities to edit the CPT
    • The CPT has enabled the show_ui setting

To prevent errors and optimize performance, JavaScript files for core and/or CPT commands will only be enqueued when needed.

Testing Instructions

  1. Ensure you're using WordPress 6.3+ (Commands API was introduced in 6.3)
  2. Ensure you have registered at least one CPT (e.g., "Books")
  3. Press Cmd+K (Mac) or Ctrl+K (Windows) in the Block Editor
  4. Type "SCF" or "Taxonomies" to see SCF-related admin commands
  5. Type your CPT name ("Books") to see its related commands ("View books", "Add book")
  6. Verify that commands correctly navigate to their respective pages
  7. As an administrator, verify you can see both admin commands and CPT ones
  8. As a user with limited permissions, verify that you only see commands you have access to. For example, an editor should only see CPT commands
  9. Test with a WordPress version earlier than 6.3 to confirm that no errors occur

Recording

Grabacion.de.pantalla.2025-04-25.a.las.13.55.21.mov

Followups

  • Consider registering commands with useCommandLoader instead, which was introduced in WP 6.4.
  • Add tests
  • Implement a new REST endpoint for CPTs to avoid using acf_localize_data
  • Migrate to TypeScript

@priethor priethor added the [Type] Enhancement New feature or request label Apr 25, 2025
@priethor priethor self-assigned this Apr 25, 2025
@priethor priethor changed the title First attempt at adding command palette support Add Command Palette support Apr 25, 2025
Copy link
Member

@gziolo gziolo left a 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.

@gziolo
Copy link
Member

gziolo commented Apr 25, 2025

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:

  • Refactoring JS files to TS.
  • Enhancing asset management to automatically detect dependencies in the build step.

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.

/**
* Register admin commands for SCF
*/
queue.add( context, () => {

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.

Copy link
Contributor Author

@priethor priethor Apr 28, 2025

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.

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.

Copy link
Contributor Author

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.

Copy link

@youknowriad youknowriad left a 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.

@youknowriad
Copy link

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.

@priethor
Copy link
Contributor Author

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.

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?

@youknowriad
Copy link

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 acf script dependency, it seems this is not needed at all and we can just use any random global variable.

$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'] );
Copy link

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?

Copy link
Contributor Author

@priethor priethor May 12, 2025

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.

@mcsf
Copy link

mcsf commented May 12, 2025

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.

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.

@priethor
Copy link
Contributor Author

Thanks, I'll address the feedback and merge the PR; I just submitted the required changes to the types endpoint to migrate the Command Palette from using acf_localiza_data to fetching the types from the REST API.

@priethor priethor force-pushed the add-command-palette-support branch from 1c1a584 to 857b84f Compare May 14, 2025 13:17
Copy link

@mcsf mcsf left a 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 🚢

@priethor
Copy link
Contributor Author

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 all_items and add_new_items provided instead, so we don't add any translatable text and reuse the same labels as wp-admin. These already have fallbacks in case they are empty, too.

@priethor priethor merged commit be909ac into trunk May 14, 2025
5 checks passed
@kraftbj kraftbj deleted the add-command-palette-support branch May 14, 2025 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants