Update the global styles sidebar to use a navigation component #34885
Conversation
youknowriad
added
the
Global Styles
youknowriad
requested review from
ajitbohra and
chrisvanpatten
as code owners
youknowriad
requested review from
oandregal,
jorgefilipecosta,
jasmussen,
ciampo and
ntsekouras
|
Size Change: +900 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
|
Thank you for pushing the work on the Global Styles Sidebar forward!
My preference would be to try using the Navigation (and routing) components from the G2 prototype, similarly to the approach taken in #32923 (comment). I'm afraid that using the current set of Gutenberg Navigation components will make it harder for us to make changes to it in the future. This PR (and, more in general, the initial phase of this project) feels like a perfect time to test those experimental Navigation components in a real-world scenario — which ultimately will help us to understand better their advantages and their limitations and to make a better-informed decision later on. |
|
I'm not at ease with having two experimental navigation components in the codebase tbh. That said, switching from one to another doesn't sound complex to me as they are supposed to have the same feature set and we're free to brake the API. I'll try a bit tomorrow to see how far I can go. |
|
I'll dive into this a bit tomorrow, but just as a quick glance at the changeset, the CSS and CSS-in-JS changes/cleanup alone look like a big improvement. |
| /> | ||
| </NavigationMenu> | ||
| <NavigationMenu menu="blocks" parentMenu="root"> | ||
| { map( blocks, ( _, name ) => ( |
There was a problem hiding this comment.
Previously we were sorting blocks alphabetically. Should we still do it?
There was a problem hiding this comment.
Should they follow the inserter's order?
There was a problem hiding this comment.
Yes, inserter order, and possibly with categories down the road.
There was a problem hiding this comment.
We'll need a search box too for quick finding.
The idea is for the newly introduced set of Navigation/Routing components to be private/local to the Global Styles sidebar, without being exposed to other parts of the repo.
I think that the two sets of components, despite they try to achieve similar results, have a quite different approach:
Of course, this doesn't mean that refactoring from one approach to another one is going to be impossible. But the previous comment from @jorgefilipecosta is a great example of the point I'm trying to make:
The G2 Global Styles prototype (which includes the g2 Navigation/Routing components) already achieves most of the requirements from the latest designs. If we pick the existing set of Gutenberg Navigation components and start iterating on them, we won't have a way of trying the G2 set of components out. By working iteratively on the Gutenberg set of components, we will slowly consolidate them and make it harder to introduce an alternative approach as time passes. Also, I don't think that a set of Navigation/Routing components should try to achieve a certain design — they should instead focus on the navigation logic, and be easily extensible by the part of the App which consumes them (this part would be in charge of styling the components)
Thank you! Let's take it from there. |
|
Coming at this from the purely visual angle, from what I can tell, the changes so far are primarily related to the resting color of the link items. Trunk: This branch:
Links in both of the above have plenty of contrast (5+:1), but since @jameskoster worked on the design, I'd love a quick sanity check on this change. |
| @@ -47,16 +45,19 @@ export const MenuUI = styled.div` | |||
|
|
|||
| export const MenuBackButtonUI = styled( Button )` | |||
| &.is-tertiary { | |||
| color: ${ G2.lightGray.ui }; | |||
| color: inherit; | |||
| opacity: 0.85; | |||
There was a problem hiding this comment.
It appears the trick here is to use opacities to define the grays, so that colors can be inherited from context instead of explicitly set to light or dark. I suspect that can work as a holdover, but it inherently becomes a bit more fuzzy, so I'd love for us to be able to feed it some specific colors.
There was a problem hiding this comment.
Yes, that's the solution. I don't think we need to be more clever than that for the moment. Adding explicit colors (and modes dark/light) is going to be harder and that component is supposed to go away anyway.
There was a problem hiding this comment.
If we can go back and tune the colors once we're happy with the overall component, it seems fine to go with the opacity solution in the mean time 👍 👍
I don't think leaving it private is any different than another experimental component, another experimental component is actually better since it allows us to start migrating the usage of the current one eventually removing it. |
To be clear here, I like this approach better (generic declarative approach) and leave the UI bits to the existing UI components. I'm just trying to be tactical and see what's best to move forward. |
|
@ciampo I've given the Navigator a try and there are a few things that we should do before trying to use it IMO. The important one is that It's tied to the browser history/url right now and for me this is a big blocker as you won't be able to use multiple ones on the same page. That said, I think migration from one to another won't be complex (approximatively): 1- transform "menuIds" to "paths" |
|
Can I have another look on the design at the moment. I think it's becoming decent enough to consider it an improvement over trunk. Here are my current thoughts: I agree with @ciampo that the current Navigation component are not great and too specific to the "menus" use-case. They don't allow some design specifics (or not easily without hacking CSS). We should definitely consider moving to the new one but the new one doesn't seem ready yet for me and would require work (especially on the router site to break the dependency towards browser history and url) That said, the current global styles sidebar on trunk is not in an acceptable state for WordPress 5.9 and we need to get the ball rolling feature wise. Moving to a navigation system will allow us to iterate on the other smaller parts (color picker, typography and layout panels) in context which is always easier while also working on bringing the new Navigation components in, in parallel. |
|
Overall, it looks like there are giant steps forward on the path to #34574: As noted, the hack that allows color inheritance through opacity isn't the greatest: item colors in the W menu are too bright, and items in the global styles menu are too light gray. But as a tradeoff in moving global styles forward, it feels worth it, provided we come back to revisit this and correct the button colors. Should we ticket it? Separately, the double border at the top of the GS sidebar would be very nice to remove soon, this one: And the subheadings in the W menu are missing correct spacing, font size and font weight:
Those are existing issues, but as the other pieces start moving in the right direction, they are becoming increasingly visible. |
|
Looks like I've found a way to use the new Navigator component without necessary using the URL. I'll see if I can open a separate PR to explore that which would give us more design flexibility. |
|
IMO design-wise this is a great improvement from what we currently have! I agree that we will need to use the new component, but until then this feels like the next step for now. |
|
Thank you @youknowriad for looking a bit more into the new Navigation components.
I understand the need to move this forward. It would be great if we could continue developing other areas of the stylebar (colors, typography and layout panels and UIs) while keeping in mind that the current navigation is a temporary (placeholder) solution. And as you suggested, it would be great if we kept exploring the new Navigation components in parallel, since I believe that they can provide a better (and more versatile) navigation/routing system than the current one.
This would be great. We definitely need to explore this aspect more, since ideally we'd want a navigation/routing system that can have multiple "roots" within the same page (e.g. the sidebar, the Wordpress menu, but we should also keep in mind that the Block Editor as a whole can be embedded in another app with its own routing system). Coordinating with browser history (and the URL) is definitely part of this. |
|
There are still some things to improve in the new Navigation components but #34904 builds on top of the current PR to move to these new components and offer more styling flexibility. Let's try to get the current PR in in a decent state and then we can try to land the refactor on top of it. |
|
@youknowriad not sure how you want to structure the breadcrumb navigation, but in the designs the current page is the one written on it. (We might need a tooltip that says "Back to Blocks".) When you are in Palette:
|
|
This looks good to me as a start! |
|
Just noting for reference, I see an error when loading and going to "colors" as the first item that crashes. Also when modifying typography items: |
|
Can I get an approval here? |
Iirc we made the links a bit darker to better contrast them against the headings which are otherwise very similar in appearance. Particularly when a description isn't present on the links:
If adjusting the colors is a pain, maybe we can do something with the type treatment of the headings instead? (It looks like there has been a regression there anyway, I'm sure they used to be larger and heavier 🤔) My only other comment is that the gray-on-blue for the active link looks a bit muddy versus the white-on-blue on trunk. |
|
@jameskoster we can always add temporary CSS to override the navigation defaults for the site editor sidebar. What could this look like? |
youknowriad
force-pushed
the
try/background-independent-navigation-component
branch
from
a66299e to
6dc5e41
Compare
|
@youknowriad if we fix the headings (I can do that separately – doesn't need to hold this PR up) then the only thing we should address here imo is the color of the text on the active state.
This text should be white instead of gray. Opened an issue here for the headings. |
|
Ok, I've made some updates, I think the balance is better now. |
youknowriad
deleted the
try/background-independent-navigation-component
branch
|
|
||
| if ( typeof blocks !== 'object' || ! root ) { | ||
| // No sidebar is shown. |
There was a problem hiding this comment.
@youknowriad 👋 This was for preventing the GS sidebar from loading if the theme didn't have a theme.json (including themes that have a experimental-theme.json file, which we don't backfill to theme.json). We now load the global sidebar for all themes, resulting in a weird situation: all panels are empty, clicking the color panel will prompts an error, clicking "reset to defaults" (for a theme with experimental-theme.json that was used in the past to store data) will also prompts an error, etc.
What can we do now to hide this sidebar if the theme does not have a theme.json?
There was a problem hiding this comment.
I have trouble understanding in which case this happen? This is for FSE themes only which are supposed to have theme.json. If they don't have theme.json, I expect FSE themes to just use the default one?
There was a problem hiding this comment.
I can share the rationale we had at the time: if a theme doesn't have a theme.json, the sidebar is hidden, no matter whether they have block templates or not. The reason for this is that if the theme does not have a theme.json we don't know whether the user styles are going to play well with it.
There was a problem hiding this comment.
I can see the logic especially for classic themes but maybe not for FSE themes and right now the site editor (the sidebar) is only for FSE themes where everything is blocks. WDYT
There was a problem hiding this comment.
For context, I was running into an issue where the wp_global_styles CPT was not registered and the editor was breaking. I think we should guard against the presence of the CPT better, because there could be different reasons why it's not there.
There was a problem hiding this comment.
Right, the CPT is only registered when the theme has a theme.json. If we change how the sidebar behaves, we should also update that logic. Something we can do is to compute the logic only in the server and pass the client a flag, instead of computing it in the client as well: that way we only need to change one place.
As for when this should be available, I'd be inclined to only load the sidebar when the theme has a theme.json. Though, I don't see any blocker to make it work like Riad mentioned (theme.json or block templates): it can be done, we just need to update the server logic to make the user CPT available also in that case.
There was a problem hiding this comment.
the CPT is only registered when the theme has a theme.json.
I didn't know about that, it makes sense to unify the check and I'd vote for (theme.json or FSE theme) personally.
There was a problem hiding this comment.
PR to centralize the logic and clarify how this should work at #35427
This is still a bit ugly but it moves us forward a bit in #34574
Here's the idea:
Based on this:
This still needs a bit of work to get something shippable as an iteration but I wanted to share to get some eyes on this.
I think it needs:
WDYT