-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Navigation: Try fixing link color in some TT2 contexts. #44578
Conversation
Size Change: +24 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
Test results:Submenu block:Both background and text color works in the editor and front. I tested both headers in TT2: the homepage and the inner pages. Page list with submenus:Tested with two levels of "child pages". Editor: With submenu and overlay text color from the palette: With submenu and overlay background color from the palette: With submenu and overlay text color that is user selected, with the color picker: With submenu and overlay background color that is user selected, with the color picker: Note, the page list currently does not fetch the custom overlay colors from the context of the parent navigation block, With background color on the navigation block, but not the submenu: In this image, the page list is the "sample page" after the search block, and when expanded, It may be easier to see if I move the page list to the first position in the menu: With background and text color on the navigation block, but no color on the the submenu: Without background, but text color on the navigation block, and no color on the submenu: |
Thank you for the extensive and thorough testing 🙏 ! It's been a little bit since I was deep into navigation, submenus, and page list, so outstanding issues and bugs separate from the TT2 issue aren't top of mind. Would you say this is an improvement over trunk, or does it need further work? |
If you tell me
Then I will take you literally :) With the exception of the page list user-selected colors from the color picker, these issues were unknown to me on trunk, but found here. So someone needs to do a full test on trunk and determine if this is an improvement or not. |
I still don't see the white text on white background on the front, note that I am testing with the 6.1 beta. It is not clear to me why the change needs to be in style.scss and not in the editor style only. |
8afc49f
to
4f2dbf9
Compare
Picking this one up again. And happy to note that this PR also fixes #20017. |
Thank you for all the thorough testing Carolina! As a result, I know more about what's going on here. There are a few things at play. First off: navigation items links ( That way, all children inherit it, so you don't have to change the color for every one of them. Secondly, global styles allows you to define a general link color, which should then (unless otherwise defined) be inherited everywhere. This has high specificity due to some Thirdly, a containing group can have link colors defined, which should then be inherited inside, unless otherwise specified. This is the TT2 issue, where a black-background group with white text causes white text inside the submenus. None of this work correctly in trunk. Shown in this GIF, a navigation block with background and text colors defined, still inheriting the wrong color from the parent group, but only in the editor: This branch fixes all of these. Showing how navigation text color affects the navigation menu items inside, unless the text color of the parent group is set, which then overrides it both in editor and frontend: That is the main reason for the presence of the code in style.scss, so that menu items are correctly and consistently inheritin the text color of the parent (important, because nav items are not always I don't know a batter fix than this, to be honest. What do you think? |
I think we should merge it :) |
Cool, I'll land this after the checks pass, but I'll keep an eye out for any followups that might happen. Worst case, we can revert! Thank you. |
An interesting aside to this is that ultimately we need to migrate the Nav block to use the standards-based block supports for colors. When we do that we'll need to reduce the specificity of the selector introduced here to ensure link styles outputted by Global Styles are more specific than inherited text styles. |
@jasmussen Unfortunately it seems that this problem has resurfaced https://core.trac.wordpress.org/ticket/57113 |
What?
Alternative to #44409. Fixes an issue where submenus in TT2 would be white on a white background.
I'm seeing this issue both for custom menus, and for page list menus, and in both the editor and the frontend. Steps to reproduce on #44409.
It isn't entirely clear to me what sets the color to white in TT2, but it is set with somewhat high specificity, so this PR needs to boost it further. It's important to test this one with all configurations of navigation colors, with and without backgrounds, with and without submenu colors.
Before:
After:
Also fixes #20017, where a link color defined in global styles can override an explicitly set color on the navigation block. Before:
After: