Size Change: +919 B (0%)

Total Size: 1.07 MB

_{compressed-size-action}

@jasmussen - One thought I had about this is that the color menu becomes really big. We could hide the overlay color options until there are submenus, but that would also make the color options depend on the state of the block tree. I'm worried that would be confusing. Do you have any thoughts on whether it's better to make the overlay colors dynamic or always there?

This is awesome! Thanks so much for working on this. This is what I see:

This is working well, except for two things:

1. When you set custom text or background colors for the overlay menu, they don't work. I didn't quite figure out why, but in case I have a moment later, I'll take a look.
2. We should change the text labels. For the menu overall, the labels should be Text and Background, and for overlays it should be Overlay text and Overlay background.

There's a separate bug I appear to have introduced, where hovering submenus when the parent doesn't have a background isn't as easy as it used to. I'll fix this separately. Similarly, the new margin/padding rules that I introduced which zero out the paddings when a background color is present that needs a little massaging:

I will take a look at that too when I have a moment.

To your question:

One thought I had about this is that the color menu becomes really big. We could hide the overlay color options until there are submenus, but that would also make the color options depend on the state of the block tree. I'm worried that would be confusing. Do you have any thoughts on whether it's better to make the overlay colors dynamic or always there?

Yes, we should not fix that here. Or to put it differently, it's okay that this menu is big because the issue has to be fixed separately. Specifically as part of #27473 (comment) and #27331

After your latest rebase and tweaks, I see this:

#31195 contains a fix for the submenu offset shown at the end of the GIF, so that's fixed separately.

While I do agree about the sidebar colors, I remain convinced that it is something we should fix separately as part of #27473 or #27331. @gziolo would you agree?

Given that, I think this one is ready for review!

While I do agree about the sidebar colors, I remain convinced that it is something we should fix separately as part of #27473 or #27331. @gziolo would you agree?

That's a good point. When there is more granular control over all settings we call Global Styles, it will be easier to reason what to render based on the current conditions, or maybe to gray out parts that don't apply at the moment.

I only glanced at the code, but It seems to look good. I like the direction 👍🏻

Just to be clear, what I was hoping to revisit separately was to, instead of showing this:

Then show a different interface for selecting colors, something more like this:

I'm going to do what I can to get contrast checks working, then I'll get this ready to merge.

Contrast checking is kinda working, but there's no good way to indicate which colors are actually conflicting without rebuilding the whole color panel.

That would be a pretty big change and a lot of duplicate code. Is it better to remove contrast checking or ship it like this while we wait for #27331?

Great questions. I see these as indications that we need to get started on the unified color picker very soon.

From testing it, to me it looks like contrast checking between nav foreground and background is working, but not overlay foreground and background:

yes it would be nice if the contrast checker message was shown more in context of the two colors it's comparing, such as below colors 1 and 2. An alternative would be to show it above, but then we'd want to rephrase to be less verbose as it's currently a very big message.

If there are no regressions compared to trunk, I think this feature is totally fine to ship as is, especially if we create followup tickets for issues and crosslink them well with the tickets noted. What do you think?

@jasmussen I am able to get both contrast warnings to appear. The only scenario I can think of where it wouldn't trigger is if you don't have any submenus in the navigation, because it needs a DOM node to get the computed style. If you do have submenus in the nav you're testing, I would appreciate hearing about that.

We also cannot move the contrast checker's position, due to how PanelColorSettings is implemented, so we can't put one above and one below or between the colors. PanelColorSettings renders the ContrastChecker as a child. It might be possible to rework the message to add a subject, e.g. "The overlay color combination may be hard for people to read" instead of "This color combination may be hard for people to read"

This is ready for review, I'll merge when I get the ✅ and create issues for:

• Improve contrast messaging
• Show contrast checker on overlay without needing a submenu in the nav
• Revisit color controls once the core controls improve

Am I missing any?

Great call. I had submenus that were generated by the Page List:

In that case, it doesn't work.

But if I convert them:

I'm not seeing the overlay colors take effect on the frontend, and I'm seeing an issue where it isn't working with the Page List block.

I'm running into a blocker with getting the Page List to render the colors. Right now I'm using the context to render classes/inline CSS and it works fine on the rendered page. However, the way the editor renders dynamic blocks means that we don't get the context. The result is that in the Page List block, the submenus inherit the colors.

This problem has been brought up in #28684, which was inspired by the Page List block.

Should we clean this up and ship it as-is, with the submenu colors working in the context of Page Link blocks in both editor and front-end, and Page List blocks on the front-end?

I considered CSS vars, but they won't work for theme colors' class names. My other thought is exploring some way to convert the context into attributes so that when the editor requests the block, it will get the colors from the context.

Looking into this, not sure I can help fix, but I'd like to try, and in that process I rebased and pushed. I'll return.

Alright I see the problem.

We are applying things like "has-yellow-background" classes directly onto the submenu items — which is appropriate enough — but that breaks with the Page List block because that's a server side rendered block mostly.

Two ideas for moving forward, I'll let you all decide what's best:

• Rewrite Page List as a client-block per advice here. Kind of a reboot.
• Change the approach here to, instead of assigning classes to submenus, instead assigning a "has-yellow-overlay-background-color" on the navigation block itself. That way it can be inherited.

What do you think? CC: @gwwar

I'm leaning towards client block as a solution, thanks for that pointer. I'd rather take on the burden of making a responsive block instead of pushing the burden on to themers.

Cc @mkaz as we just chatted about the page list also.

@jasmussen @gwwar If I can get the client block working I'll open a PR for just that change, and then this can follow-up on that. I'm making enough progress that I think this is a good way to go forward.

Two ideas for moving forward, I'll let you all decide what's best:
Rewrite Page List as a client-block per advice here. Kind of a reboot.
Change the approach here to, instead of assigning classes to submenus, instead assigning a "has-yellow-overlay-background-color" on the navigation block itself. That way it can be inherited.

So one thing we'd need to do before attempting to rewrite page list as a client block, is doing a performance profiling pass on navigation links. Right now the editor visibility slows down with only > 50 or so which would very quickly show up on page list if we swapped implementations.

To help move this forward, since we can now transform small lists of PageList -> NavigationLinks, I think it'd be fine to reduce scope here to target NavigationLinks only.

Another option is to also apply the color classes to the navigation block itself. That way they can be inherited in CSS only.

@georgeh thinking about this some more, perhaps it really is best that the overlay menu color property is a property of the navigation block itself rather than each individual menu item. Because it might be best to use this same color control for the 🍔 menu background, and for that to work, it would need to be inherited.