Site Editor: create router adapter for sidebar #60466
Conversation
jsnajdr
added
[Type] Enhancement
|
Size Change: -14 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
| @@ -35,6 +37,7 @@ export default function useLayoutAreas() { | |||
| return { | |||
| key: 'pages-list', | |||
| areas: { | |||
| sidebar: <Sidebar router={ router } />, | |||
There was a problem hiding this comment.
It's unfortunate that we keep rendering a generic "Sidebar" component here. I wanted the routes to be more clear here. (Basically move the sidebar routing into this component instead)
Today it feels like we're defining routes twice.
There was a problem hiding this comment.
Good point. 🙂 I'll try to deconstruct Sidebar into individual items. That should make the Navigator stuff disappear sooner or later.
There was a problem hiding this comment.
In 06000f3 I made some progress on this. The Sidebar component is completely dissolved and all the individual items are put into area.sidebar.
There is still NavigationProvider, but it's there to:
- expose APIs used by
useNavigator(), like thegoToorgoToParentmethods, and the currentlocation. - keep the "navigator" styles on the rendered elements.
I no longer need the code that registers screens in the router: there's now one hardcoded SCREENS array used to map paths and determine parent paths.
I'll continue working on eliminating these remaining parts of the Navigator router, too.
There was a problem hiding this comment.
This already feels like a great improvement and a step in the right direction 👍
There was a problem hiding this comment.
The use of Navigator components is now almost completely eliminated, the remaining usages are now mere shadows 🙂
The items in the Site Editor sidebar are still NavigatorButtons:
They use the goTo method of useNavigator(). I'm going to remove that now, and replace goTo with history.push.
Each of the sidebar screens is also still wrapped in NavigatorScreen. It reads the location from useNavigator, but the only properties it looks at are things related to focus management. It doesn't look at the actual location at all. Focus management is the last big thing to solve in this PR, I was postponing it until now.
I'm adding @oandregal as a reviewer for awareness, because I'm noticing the work being done on eliminating /wp_template_part/all and sometimes there are merge conflicts.
There was a problem hiding this comment.
Today I eliminated even the last usages of Navigator. It's not there at all in Site Editor sidebar and routing.
Two things I need to reconstruct before this is ready for merge:
- Animations when navigating forward and back in the Sidebar. When pressing the Back button, we want the next screen to slide from the left. When pressing one of the "forward" buttons, i.e., the one of the items with chevrons, we want to slide from the right.
- Focus management. When navigating forward, focus the "back" button. When navigating backward, focus the item leading to where we came from. Pressing Enter repeatedly should navigate back and forth between the same two items.
I think both of these behaviors are local to the Sidebar, and could be handled in its local state. The global router doesn't necessarily need to know about it.
|
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 Object.entries( subset ).every( ( [ key, value ] ) => { | ||
| return superset[ key ] === value; | ||
| } ); | ||
| function getParamsFromPath( path, params ) { |
There was a problem hiding this comment.
This is better than what we had but still feels hacky. I'd love to be able to use the same useLocation/useParams/useRouter to retrieve the params from the url, regardless of where I am (sidebar, content...)
|
No rush, but it'd be nice if we could elaborate on why this refactoring was needed. Right now, the description doesn't include the context on why this is being done, apart from some hints you can get by reading between the lines. What problems did the old approach have? Is it to solve technical debt? Is there an issue with more background info we could link to? it'd be super helpful for people unfamiliar with the motivations behind this change. Thank you! 🙏🏻 |
|
@fullofcaffeine I've asked @jsnajdr to take a look at the routing situations in the site editor for a few different reasons:
you're right that I should have started with an issue outlining the issues, which I just did here #60584 |
jsnajdr
force-pushed
the
update/site-editor-one-router
branch
2 times, most recently
from
06000f3
to
c592f4c
Compare
| } | ||
|
|
||
| // Go back to root by default | ||
| return {}; |
There was a problem hiding this comment.
I'm not sure that's always true for non post type screens? It might be the case now but I don't feel like this is the right path for a generic solution. I feel like maybe the best way to solve this is to actually pass the right "back path" on each sidebar component.
Obviously not something that need to be solved now or anything, just thinking out loud.
There was a problem hiding this comment.
I think it's correct for all existing screens. I'm currently not aware of any navigation link that would point to a wrong direction.
But yes, I also think we'll end up passing an explicit backPath to every screen, instead of trying to guess it with some heuristic.
There was a problem hiding this comment.
Agree with this follow-up as well.
|
Awesome work on this PR so far. |
jsnajdr
force-pushed
the
update/site-editor-one-router
branch
from
d74bae6
to
020c5c4
Compare
|
I see you've added the animation. There's one case where the existing code fails the "manage all template parts" page which is supposed to be a child of "patterns". Noting that this page is going to be removed (merged with patterns) soon. That said, even if it is, it's something that shows where the current code doesn't adapt properly. Previous we used to rely on the "path" to know whether a path is parent or not (back animation or not), if we had pretty permalinks we could do so here too but for now, it feels like the best path is probably to be explicit in the links themselves. |
Yes, this is because my current animation code is very primitive, namely the The site editor code never really knew that navigator.goTo( backPath, { isBack: true } );and this code says explicitly that it is an This |
I pushed a fix for this. Now, when going back from "manage all template parts", the focus is at the right place.
Yes, ideally the focus should be on "manage all template parts" because that's where you came from. It has to do with using the |
|
I'm seeing two issues:
|
I still see this one, but the other bug is fixed. |
jsnajdr
force-pushed
the
update/site-editor-one-router
branch
from
3b75842
to
7490458
Compare
The unwanted focus on initial load is now also fixed. |
|
Confirmed the fix. |
|
@tyxla's feedback is mostly related to the |
|
I was testing this and looking at the code and found an issue with the template backpath. Any other entity (pages, patterns, template parts) works fine. This is how to reproduce:
Gravacao.do.ecra.2024-04-18.as.10.12.16.mov |
|
Fix at #60850 |
| canvas === 'edit' ? ( | ||
| <Editor isLoading={ isSiteEditorLoading } /> | ||
| ) : undefined, | ||
| mobile: canvas === 'edit' && ( |
There was a problem hiding this comment.
This refactoring made things a lot more simple to follow, thanks @jsnajdr!
I find one little thing missing: fully controlling the mobile views. At the moment, we only use areas.mobile for displaying components other than the sidebar. The logic to default to the sidebar if areas.mobile is undefined still lives in the layout. Absorbing as part of the router would simplify things further.
First working version of a consolidated router for Site Editor sidebar.
The
NavigatorProviderinSidebarno longer has its own in-memory router synchronized with the browser history withuseSyncPathWithURL.Instead, we create a "router adapter" around the native browser history, and pass it as a
routerprop toSidebarand in turn toNavigatorProvider. Theroutervalue is identical to the context value that is then passed down inNavigatorContext.This router adapter currently duplicates a lot of code from the
NavigatorProvider. The reducer to register screens, the code for matching paths and finding parent paths. After we add all the missing features togoToandgoToParent(focus management etc.), there will be more duplication. We'll have to solve that somehow.Currently focus management (focusing after navigation and after going back) is not implemented, I'll work on that further.
The sole source of truth for the current location and its history is the global browser history. The router adapter doesn't store its own history state. It merely maps the global location params to the
SidebarURL scheme. And then maps it in the opposite direction in methods likegoTo, which maps the path and then acts on the browser history directly.