Navigation Block: Add Color Options for Submenus #31149
Conversation
|
Size Change: +919 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
@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:
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:
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 |
georgeh
force-pushed
the
add/navigation-block-overlay-color
branch
from
d325c5d
to
14401c8
Compare
|
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! |
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? |
georgeh
force-pushed
the
add/navigation-block-overlay-color
branch
from
6f0e6b4
to
81f006c
Compare
|
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 This is ready for review, I'll merge when I get the ✅ and create issues for:
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. |
jasmussen
force-pushed
the
add/navigation-block-overlay-color
branch
from
fd59864
to
62396e3
Compare
|
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:
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. |
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. |
This reverts commit 048f9e1.
Limitations: - doesn't seem to support all theme colors, just very basic colors - does not support custom colors (I don't think there is a place to store custom CSS, see wp_setup_nav_menu_item() - may have trouble with translating classes like-has 5-red-color to 5red because we don't know if the hyphen was inserted by us or the theme
georgeh
force-pushed
the
add/navigation-block-overlay-color
branch
from
4572b2b
to
2be2b54
Compare
|
Just wanted to mentioned that this PR seems to have introduced an issue with undo/redo that's tracked in #34564. It's related to the way the |
Closes #29963
Description
Adds options to color the submenus separately from the main navigation block
TODO
How has this been tested?
Manually tested
Screenshots
nav.colors.mov
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).