Add link color support to Nav block to allow for override of Theme JSON styles #41199
Conversation
|
Size Change: +639 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
I'm not sure about this approach. It seems to me that the behaviour in the editor is the correct behaviour. In CSS if I set a link color in an element then to override it I need to set a different link color in the child - setting a text color in the child won't be enough. I think what we need to do is to make it possible to set a link color in the nav block, rather than a text color. This is a bit confusing though as I'm not sure its possible to have text in a navigation block which isn't a link, although one day it probably will be. |
Yeh you're right. It's the link color not the text color that's overwriting the block specific styles. Ok let's add linkcolor support to the Nav block. |
|
@scruffian I added |
|
@scruffian I've noticed the CSS specificity is different in the editor on the Global Styles selector due to it being prepended with |
| @@ -41,8 +41,6 @@ $navigation-icon-size: 24px; | |||
| } | |||
|
|
|||
| // Menu item link. | |||
| // By adding low specificity, we enable compatibility with link colors set in theme.json, | |||
| // but still allow them to be overridden by user-set colors. | |||
| .wp-block-navigation-item__content { | |||
| color: inherit; | |||
There was a problem hiding this comment.
I think we should remove this line. .wp-block-navigation-item__content is a class that is used on a link. This line forces links to inherit their color from their parent which is against the pattern of CSS.
|
Sorry, I'm not sure the approach here is the right one. The elements API expects the link color to be stored in a very specific attribute: gutenberg/lib/block-supports/elements.php Line 98 in d36ee96 If we do this then we won't need to write any custom code to apply the colors, as the library should take care of that for us. |
I'm pretty sure the Nav block doesn't follow "the rules" here. All the stuff for text and background color is custom coded. I think this is because the colours aren't intended for the Nav block itself but rather to be inherited by the child blocks. I can try uses the standard approach you suggest but I deliberately avoided this and followed the patterns that have already been established in the block (e.g. |
|
Some more research reveals that the need for custom colors for the If I add support for "supports": {
"color": {
"link": true
}
}...then I get x2 color controls:
In the longer term we may need a wider refactor of how Submenu and Overlay colors are handled. For now though in order to fix this bug I think building on the existing custom implementation would be fine. |
|
Other options: Separate Submenu & Overlay colors panelWe can use standard block supports for the text, link and background colors. Then we can have a dedicated panel for the "custom" color controls for Submenu and Overlay.
|
|
Ok, this one is going to get complicated. My suggestion is:
|
There was a problem hiding this comment.
I believe this is a good opportunity to not patch to fix. If I am not mistaken we'd be introducing things that we'll need to remove later.
We already have a system in place and we should try to improve that one for this specific use case:
- properties set on parents should be inherited by children, unless children override them
As far as I understand the global styles system supports this just fine, but the navigation block has a custom implementation. Maybe it's time to remove that and make sure we fit into what the platform currently offers? Probably at the time the global styles system did not offer us a way to implement what we wanted, or whatever was the reason.
I understand that refactoring is a big undertaking and the bug is now, but fixing will require us to support temporary solutions and remove them later, they're not even experimental. Also, I believe the fix now refactor later should only apply to core on critical bugs. Is this color inheritance on one block a critical bug?
| @@ -43,10 +43,14 @@ | |||
| "usesContext": [ | |||
| "textColor", | |||
| "customTextColor", | |||
| "linkColor", | |||
| "customLinlColor", | |||
There was a problem hiding this comment.
| "customLinlColor", | |
| "customLinkColor", |
|
Thanks for your review @draganescu . It's much appreciated.
You are correct. The question is, how much are we really compounding the problem vs being able to move quickly to patch a fix.
Global Styles has block supports for The Nav block has a custom implementation for these things because we need to support custom color setitngs for overlay and submenu as per #41199 (comment).
I think we owe it to the original reporters and our Themes community to check on the impact of this bug. If it's something that can be "managed" we can go straight to the refactor. If it's critical then we'll need to patch and then refactor. I would also prefer not to compound the existing tech debt 👍 |
|
Closing this one as I have a new plan. I will raise a PR shortly. |
What?
Fixes #41146 by adding
linkColorsupport to the Nav block thereby ensuring that Nav block specific link color styles can take precedence over inherited Theme JSON styles.Why?
In #41146 (comment) we learnt that the Theme JSON styles coming from link color styles on the enclosing Group block (the one wrapping the Nav block) were taking precedence over the user selected text color styles on the Nav block.
This is correct (if unintuative) behaviour as CSS cascade means the link selector from the Global Styles will target the
<a>of the Nav block. That overrides the text color set on the Nav block.This is becayse textColor !== linkColor.
We need to provide a way for the Nav block to overide linkColor styles cascading from Theme JSON.
How?
This PR:
linkColorandcustomLinkColorsupport to the Navigation block.inherited from the appropriate parent on to the appropriate element.The above requires jumping through many hoops and thus why there are so many file changes.
Alternatives considered
Initially this PR targetted the existing
textColoron the Nav block and ensured that took precedence on any styles cascading from Global Styles. However as @scruffian pointed out that is counterintuitive as link and text color are not the same thing.One concern here is that for a while Nav block consumers have probably seen the
Textcolor control as equivalent to "the color of the links in the Nav". However in the future we may have text items in a Nav which are not links and so we need to start to differentiate them.Testing Instructions
Overlay and submenu linkcolor on the Nav block. Make sure this distinct from the previous color and the one on the Group block.🤔 We should also test that adding styles from Theme JSON that specifically target the Nav block are still respected.
Screenshots or screencast
Screen.Capture.on.2022-05-20.at.12-42-47.mp4