Try: Remove color inheritance specificity. #35725
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasmussen
added
[Block] Navigation
|
Size Change: +98 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Thanks so much for the green light. I'm tempted to land this because it's such a small change. However it's at a sensitive place in the code, so I'd love to just boost the confidence level even more with a few more reviews, if people have time. Thanks! |
|
Thanks a lot, @jasmussen! I'm seeing the following with TT1 Blocks. This obviously seems fine for the Site Title; I am however not entirely sure about the navigation links. Are they supposed to respect the text color, i.e. should they be red (as the 3rd screenshot in the PR description seems to indicate)?
|
|
Ah, the navigation links are red on the frontend:
|
ockham
requested changes
Oct 18, 2021
|
Yes, excellent catch, I just verified that I see this in trunk: And this in this branch: I knew it was too good to be true 🙈 but it is encouraging that things work as they should on the frontend, that's a tip for a fix, probably. |
|
So just to chalk up the problem, it appears that this editor style (or theme.json property): ... wins over the rule we tweaked:
... to inherit this inline style attribute:
So at least that reminded me why I mentioned needing to override a value from "global styles". I'll need to find a different path forward here, and I'll put my thinking hat on. |
|
Alright, I tried targetting a classname instead of an element, to carefuly thread the specificity needle if at all possible. So it's now I had to move a padding rule as well due to a "no-duplicate-selectors" eslint rule catching me. And I'd still appreciate a good testing for this one. It's not a lot of code, but it's a careful balance. |
ockham
approved these changes
Oct 18, 2021
|
Sweet, thank you, and thanks everyone. I'm going to test this one a bit more tomorrow, then land it. |
|
I tested a rebased version of this with TT1 blocks again, empty theme, and Blockbase, and things still appear to work. So I'm going to land this one as is, but I would appreciate those of you receiving this message to help keep an eye out for any text-color/global styles related navigation block issues that might have resulted from this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Probably fixes #34648.
#31878 heavily refactored the navigation block CSS to reduce specificity of many selectors, so as to allow global styles to override colors. One part of that was also to override inherited colors. I don't recall the specifics, but this was necessary at the time to fix an issue with colors otherwise not inheriting at all. As a result, if you insert a Site Title block inside navigation, and customize its link color, that doesn't work:
That extra specificity doesn't appear to be necessary anymore. And in removing it, things appear to work as they should:
Because the rule was necessary at the time, but doesn't appear to be necessary anymore, it would be good to test this one well.
How has this been tested?
Here's some test content:
Please test in a few themes, including Empty Theme and some block themes. In combination with having a site title with a customized link color, please test most of the navigation properties and features, including all the color options, most notably submenus and the mobile responsive overlay menu, and verify that the colors defined on the navigation block apply all the way down the hierarchy, unless overridden by an individual block.
Checklist:
*.native.jsfiles for terms that need renaming or removal).