Extract the WordPress reusable commands to a dedicated package #49956
Conversation
youknowriad
requested review from
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
| 'site-editor.php', | ||
| args | ||
| ); | ||
| document.location = targetUrl; |
There was a problem hiding this comment.
Here I've tried using window.history but failed to trigger the site editor to reload its content. I'm not sure if there's a way to trigger "client site routing" without having access to the "history" object. Anyone knows?
|
Size Change: -778 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
draganescu
changed the title
There was a problem hiding this comment.
It makes sense to extract the commands from edit-site.
I don't have a strong opinion here: I understand the motivation for creating a new package, but I wouldn't have opposed keeping everything in commands at this early stage, until we have a clearer picture of how to organise everything.
|
@jasmussen Can you test this PR and specifically, navigating to pages/templates... and do you think it's acceptable for now to do a full reload? The only solution I can see to this problem is to actually move the "history/router" to a new dedicated package outside of |
Sure, took it for a spin. Here's a GIF of navigating to templates, navigation, etc:
It isn't entirely clear here what I'm supposed to look for in terms of a full page reload. The closest I could see what when I navigated to Templates > Manage all templates, and then seeing a brief white flash when navigating back. Is that the full reload you're referring to? I'm not seeing it in trunk. If yes, then I have to admit this makes me a little nervous because it's a white flash, and is exactly what a reacty application like this can avoid. Thinking ahead, one of our goals for 6.3 is to make the editor feel really polished, resilient, and solid. Just one trivial aspect of that is to ensure an initial progressbar loading state for the site editor, and unified loading states across all subpages (the skeleton flashing in Navigation for example doesn't seem to match anything we use anywhere else). I know those aspects are orthogonal to what you're asking. But I bring them up because they are subtle details which embrace the appy nature, whereas a white flash, even if brief, is one of the things we want to avoid by adding such loaders. |
|
No, I didn't touch navigation from the sidebar at all here, I was referring to the navigation from the command menu (cmd + k) |
|
Apologies, took it for another spin:
I'm not really experiencing anything here that feels off to me. Anything in particular I should be on the look out for? Only one issue, which seems unrelated to this PR: if I press ⌘K and press Escape, then I can't press ⌘K again until I click somewhere in the dark material. It seems focus is moved to a place where the shortcut doesn't work, perhaps? |
|
@jasmussen did you try navigating to say a template "index" from the command menu? |
|
Oh I see the blink now:
As I noted when I thought the blink was about something else, I think this is the type of thing to mainly avoid if at all possible. Even if it's a super subtle literally blink and you'll miss it thing, it contributes a little bit to a feeling of being not as considered as it should be. If it's a temporary thing, it seems fine. But ideally the commandbar can be a little aware that it's already in the main section, when it navigates to a sub-section. Or to put it even simpler — we mainly want to avoid those blinks. If we can make those blinks invisible or not happen, that's really the main thing. |
|
I guess in that case, I see two options: 1- Extract the "router/history" from the site editor. So this means probably a new package (to be honest I considered extracting this before). It can be all private APIs for now. What do you think @mcsf ? I was thinking of trying the first option but open to suggestions. |
I would say that extracting that logic and having consistent router support across contexts is inevitably what we'll need to do sooner or later (also see Phase 3 and WP-Admin modernisation). That said, it's more than just extracting to a new package, right? If I'm not mistaken, this would mean changing from "one page = one editor containing one router" to "one page = one supervisor + router capable of loading different editors", no? |
Not really, the idea here is to just to have the router API (consuming it from the context essentially) as an independent package. It will still be only used in the site editor and the command could rely on it to navigate in the site editor and fallback to regular navigation in any other wp-admin page. |
|
Ah right, I was getting ahead of myself, thinking about integrating routing/history across our editors. 😅 Yes, that sounds good. 👍 |
An equivalent to Core's |
youknowriad
force-pushed
the
update/extra-commands-to-reusable-package
branch
from
df6d2ae
to
1d09653
Compare
|
@jasmussen I extracted the router package so now I restored the frontend navigation (like it is in trunk) |
|
Need a test? |
|
Always :P |
|
I guess I forgot that I checked the "auto-merge" checkbox :P |
That would work. We took a similar approach for sharing URLs for nonce or REST API endpoints: We also hardcoded some paths in the code a long time ago, examples: |
Related #48457
What?
We want the command menu to be available in multiple WP Admin pages (if not all of WP Admin) but we don't want to writing the same commands over and over again. This PR extracts the commands we currently have in the site editor (creating post and pages and navigation in the site editor) into a dedicated
@wordpress/core-commandspackage to be reused in multiple places. That package shouldn't be depending on the "edit-site" package.At the moment, I didn't add the command menu UI to any new page, but after this PR lands, it's my next thing to tackle.
Important notes
Testing Instructions
1- Open the site editor and focus the page.
2- Click cmd + k.
3- Use the command menu. (no big changes here)