Site Editor: Prepare route registration by refactoring the site editor router #66030
Conversation
youknowriad
added
the
[Feature] Site Editor
|
Size Change: +465 B (+0.03%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1509763. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11343031250
|
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
a team,
oandregal and
nerrad
and removed request for
a team
There was a problem hiding this comment.
I am liking the direction of this centralized route registration! Great step to allow plugins etc to extend the site editor's routing capabilities.
I navigated back and forth between all sidebars routes, and, aside from the animations could not see any regressions.
There was one teensy difference between trunk and this PR: on the pages route, the data view panel is much wider.
| Trunk | This PR |
|---|---|
 |
 |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| return ( | ||
| params.postType === NAVIGATION_POST_TYPE && | ||
| ! params.postId && | ||
| params.canvas === 'edit' |
There was a problem hiding this comment.
Does the canvas mode really warrant a separate route? Aside from the mobile view it still renders the same sidebar and the same <Editor /> component. The canvas mode is managed by the canvasMode state in the data store and synced by the useSyncCanvasModeWithURL.
In my view the canvas mode should be eventually managed only at one place; either:
a) in the route handler, which passes the mode down as <Editor canvasMode={ params.canvas } /> prop;
b) inside the <Editor /> component which reads the mode from the URL itself. This setup is very similar to how you would work with "minor" query params like order=asc or search=foo
There was a problem hiding this comment.
No disagreement, the main reason for the route is "mobile". I don't know how to handle it otherwise. For mobile, these routes render different things depending on the canvasMode. Not entirely sure how to handle it.
Aside: I would love to remove that duplicate state as well and only rely on the url but that is a separate large effort IMO.
There was a problem hiding this comment.
I agree this is worth revisiting, although I should point out that I like the rigidity of the current proposal: other than the match predicate, everything else is static data, making the whole thing IMO very easy to keep track of. It's more difficult for surprising behaviour to creep in, especially once we start considering third-party use. (Note that this remark doesn't imply that addressing the mobile view in a smarter way would necessarily go against the principle.)
| function routes( state = [], action ) { | ||
| switch ( action.type ) { | ||
| case 'REGISTER_ROUTE': | ||
| return [ ...state, action.route ]; |
There was a problem hiding this comment.
We should probably also have a method to register multiple routes at once. Instead of having to go through registry.batch. Plugins will use it all the time.
Do we ever need to unregister a route?
There was a problem hiding this comment.
Unregistering seems like a good thing to have to potentially override a route or something.
Would love that personally. |
|
Sorry this is a high-level question that doesn't involve the code. Please ignore me if this has already been discussed elsewhere! Do you have any plans for integrating (or make compatible with) other third-party routing libraries in case we need more features? Even if we don't need it, I think it's nice to be aligned with popular libraries' API so that we know what we might run into in the future. Copying the API has some other advantages:
|
Actually, this PR started locally by using a third-party router library (I tried both React Router and TanStack Router), I agree entirely that we should try using some of these libraries to avoid recreating the same feature over and over again. That said, here's what I found:
That said, as I said initially I did try to use one of the libraries to replace These libraries are not easily adaptable to the use-case of multiple routable areas (sidebar, preview, content). The work by nesting routes within a single outlet (previous versions of react router support named outlets I think). So when using them it meant that we'd be "forced" to avoid using the nested capabilities which means losing nice transitions... between nested routes. Some potential solutions involved using "slots" and choosing one of our areas as the "outlet" area but it's too involved and not a clear "winner" solution. Another approach would be to use multiple routers for the different areas. I think these options (and maybe others) are still on the table but I wanted to focus on our immediate need first which is to address the higher-level registration problem. |
youknowriad
force-pushed
the
update/site-editor-register-routes
branch
from
8f312a8 to
1509763
Compare
For the animations, it's actually related to this comment #66030 (comment) Previously, the animation was triggered when we changed the route key/name or when we changed the canvasValue. With the current PR, these can happen both at the same time but since the "canvas" is a redux state value, it happens slightly after the route name change causing the animation to be triggered twice kind of which breaks it entirely. I made the choice to keep the animation only for canvas change for now but I think either removing the canvas state or using the same route for both canvasMode would solve the issue. But I think it's ok for now to not have animation when switching top level routes. |
| return ( | ||
| params.postType === NAVIGATION_POST_TYPE && | ||
| ! params.postId && | ||
| params.canvas === 'edit' |
There was a problem hiding this comment.
I agree this is worth revisiting, although I should point out that I like the rigidity of the current proposal: other than the match predicate, everything else is static data, making the whole thing IMO very easy to keep track of. It's more difficult for surprising behaviour to creep in, especially once we start considering third-party use. (Note that this remark doesn't imply that addressing the mobile view in a smarter way would necessarily go against the principle.)
Co-authored-by: Miguel Fonseca <[email protected]>
| }, | ||
| areas: { | ||
| sidebar: <SidebarNavigationScreenMain />, | ||
| preview: <Editor />, |
There was a problem hiding this comment.
I had forgotten why this didn't have a mobile area until I reviewed the layout component. Prepared #66309 to document when each area is present. It also prevents the edit area from being rendered if the canvas mode is edit.
…r router (WordPress#66030) Co-authored-by: youknowriad <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: jsnajdr <[email protected]> Co-authored-by: mcsf <[email protected]> Co-authored-by: kevin940726 <[email protected]>
What?
For a long time, people have been asking about an API to register pages and routes into the site editor. We're not ready yet to open such API because there are unknowns like: How to expand this API later to the full admin... That said, the current PR makes a huge progress towards that by adding an internal registration API and refactor the existing routes to use it.
How?
A route object would be something like:
Note
This PR is not entirely ready, there are still some transitions to improve and we need to do some good testing and debugging.
Testing Instructions
Navigate everywhere in the site editor.