-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Site Editor: create router adapter for sidebar #60466
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thegoTo
orgoToParent
methods, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already feels like a great improvement and a step in the right direction 👍
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.
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 NavigatorButton
s:
![Screenshot 2024-04-15 at 9 19 41](https://private-user-images.githubusercontent.com/664258/322387462-f6c2514d-71ab-4066-a590-6e9d18d4de44.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQxMzQ3ODgsIm5iZiI6MTcyNDEzNDQ4OCwicGF0aCI6Ii82NjQyNTgvMzIyMzg3NDYyLWY2YzI1MTRkLTcxYWItNDA2Ni1hNTkwLTZlOWQxOGQ0ZGU0NC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwODIwJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDgyMFQwNjE0NDhaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT00MmY5YTI4ODIwYWM1M2ZlNjYzODIzMWJhNTA2OTdhYzA5ZjA2NzY3ZjQ4YTg5NGE4OWE4NDlmNTcxMjAxM2QyJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.GSYIbQ4TwrSPWX5bWEjqfJPi0TSQPGPXOkJS0SV_su0)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
06000f3
to
c592f4c
Compare
} | ||
|
||
// Go back to root by default | ||
return {}; |
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with this follow-up as well.
Awesome work on this PR so far. |
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 |
Other than that small bug, this is looking very good to me. |
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. |
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.
Still a tiny bug but this is looking to me. Awesome work here.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
NavigatorProvider
inSidebar
no 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
router
prop toSidebar
and in turn toNavigatorProvider
. Therouter
value 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 togoTo
andgoToParent
(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
Sidebar
URL scheme. And then maps it in the opposite direction in methods likegoTo
, which maps the path and then acts on the browser history directly.