Switch background color to text color on block separator #44943
Conversation
cbravobernal
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
cbravobernal
added
[Type] Bug
cbravobernal
requested review from
jasmussen,
glendaviesnz,
ramonjd and
tellthemachines
and removed request for
ajitbohra
|
I don't have any direct examples handy, but one of the values of using Would a potential alternative be to explore moving the styling of the separator from using a border, to using height + background color instead? The downside there is that it might not be easy to maintain backwards compatibility 😅 |
|
If we make this change we would need to add a deprecation to migrate the existing background colors to text colors, otherwise, the existing background colors override any new text colors added: separator-background.mp4Also, the change in this PR only works with preset colors. If you add a custom color, or set the alpha channel on a color it doesn't work as the custom colors are being manually applied in the edit/save functions from the background attribute: custom-color.mp4 |
ockham
added
the
Backport to WP 6.7 Beta/RC
|
The separator color is still not updated on the variation switch 😢 |
|
Size Change: +211 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work @c4rl0sbr4v0, this is testing really well! Just left a nit / discussion about where to put the PHP code, but in terms of functionality, this is working great, and plays nicely with some custom separators I've been working on for a hobby theme.
One subtlety that I noticed, is that the .is-style-dots style variation that ships with the separator block uses the color property to set the color of the dots. So, if I just set the background color in my theme.json file, then the dots style will not be targeted. However if I include text as well, then it is:
Just setting background |
Setting background and color |
|---|---|
 |
 |
Should color also be output along with border-color if color isn't already present in theme.json? As far as I know, it's only the dots style that is affected.
In the latter, I'm using the following snippet in my theme.json file under styles.blocks:
"core/separator": {
"color": {
"text": "#ff0000",
"background": "#ff0000"
}
}
Also, in case it helps for testing, here are a couple of code snippets for the custom diamond separator style in my screenshots, which uses background colors (to be clear, the diamond separator is working nicely, so need to re-test, this is included just for extra context):
PHP:
register_block_style(
'core/separator',
array(
'name' => 'diamond',
'label' => __( 'Diamond', 'darkpastel' ),
)
);CSS:
The CSS needs a couple of !important rules to override some of the core styling, and the :where() rule for background color is so that it's a lower specificity fallback that the theme.json background color can override:
.wp-block-separator.is-style-diamond {
width: 25px !important;
height: 25px !important;
border: none !important;
clip-path: polygon(50% 0%, 100% 50%, 50% 100%, 0% 50%);
}
:where( .wp-block-separator.is-style-diamond ) {
background: black;
}| 'name' => 'border-color', | ||
| 'value' => $background_matches[0]['value'], | ||
| ); | ||
| $block_rules = static::to_ruleset( $selector, $declarations ); |
There was a problem hiding this comment.
Code quality nit: this line replaces $block_rules instead of concatenating to it.
Is this because we've already handled static::to_ruleset above in // 2.?
I'm wondering if a safer approach might be to move this section to be above // 2., or even above // 1. so that the $declarations array is mutated before the static::to_ruleset output in // 2.?
My main thinking is that because elsewhere it's expected that $block_rules is always concatenated to, it could be easy to accidentally miss that on this line the value is being replaced.
Alternately, instead of appending to $declarations[], we could create a separate $declarations_separator array for this additional border-color rule, and use that directly in the ruleset output, a bit like how the duotone works. E.g. $block_rules .= static::to_ruleset( $selector_duotone, $declarations_separator );
Finally, would it be worth moving this logic to a separate function, since it adds a fair bit to the length of get_styles_for_block? Or is it too specific to a single usage for it to be worth the abstraction 🤔
There was a problem hiding this comment.
Finally, would it be worth moving this logic to a separate function, since it adds a fair bit to the length of get_styles_for_block? Or is it too specific to a single usage for it to be worth the abstraction 🤔
I tried to move it to a filter but I could not call the protected to_ruleset function. I also think is a good idea to move it, that way we could remove it in the future if we update the separator block to use only border options.
Also, I thought to try the idea of just adding 1px height if a background is defined, that could do the work and would be less invasive, but I guess it won't work for dotted separators 😢
I will update it with all the nits!
There was a problem hiding this comment.
Should color also be output along with border color if color isn't already present in theme.json? As far as I know, it's only the dots style that is affected.
I think it could be better to use only color. We don't need border color, as it seems that color is a better fit (works for both line and dotted separators)
There was a problem hiding this comment.
instead of appending to $declarations[], we could create a separate $declarations_separator array for this additional border-color rule, and use that directly in the ruleset output, a bit like how the duotone works. E.g. $block_rules .= static::to_ruleset( $selector_duotone, $declarations_separator );
I think just updating the declarations before step 2 will be more performant.
cbravobernal
changed the title
There was a problem hiding this comment.
This is testing well for me. I've tested the following aspects:
- ✅ You can use any valid CSS color values (
rgb(),rgba(),hsl(), etc.) in the theme.json as mentioned in Switch background color to text color on block separator #44943 (comment) - ✅ Works for the custom separator styles like the "Diamond" style from Switch background color to text color on block separator #44943 (review) as well as with the
The only issue I have noticed was that setting only the text color in the theme.json does not affect the color of the "custom style" separator (like in the Diamond example) but I think this is expected behaviour? See the video below for a better explanation:
2022-10-24_21-09-25.mp4
So, all in al I'd be inclined to merge it 👍
cbravobernal
force-pushed
the
fix/43660-separator-background-style-variation
branch
from
a0b26ae to
8e35090
Compare
cbravobernal
deleted the
fix/43660-separator-background-style-variation
branch
Mamaduka
added
Backport to WP Minor Release
* Switch background to text, remove gradients on separator block * Use theme_json class to fix separator color issue * Add fix for the editor * Fix the separator on the editor * Give the border color more specifity * Add text color specifity * Small refactor * Add unit test for separator background php part * Use only color instead of border-color * Refactor to just update declarations * Update documentation * Add static to private function * Update the wording of the comments * Add missing spread to global styles output * Rename test to fit the new function Co-authored-by: Michal Czaplinski <[email protected]> Co-authored-by: Andrew Serong <[email protected]>
* Switch background to text, remove gradients on separator block * Use theme_json class to fix separator color issue * Add fix for the editor * Fix the separator on the editor * Give the border color more specifity * Add text color specifity * Small refactor * Add unit test for separator background php part * Use only color instead of border-color * Refactor to just update declarations * Update documentation * Add static to private function * Update the wording of the comments * Add missing spread to global styles output * Rename test to fit the new function Co-authored-by: Michal Czaplinski <[email protected]> Co-authored-by: Andrew Serong <[email protected]>
Mamaduka
removed
the
Backport to WP Minor Release
|
I've also arrived here while looking into #57297 & #41194 and am trying to understand some aspects of this PR's approach. @cbravobernal, do you recall what drove the different approaches across PHP and JS? In the PHP theme.json class, the style declarations get fudged but on the JS side the actual global styles data tree is updated. Before I fix #57297 and the logic introduced here, I wanted to make sure I wasn't missing a reason both the JS and PHP implementations couldn't be consistent. Appreciate any light you can shed here 🙏 |
As far as I remember (I even didn't remember this issue lol), we updated the PHP implementation to be similar than the JS one, but using a filter to be able to fix previous separators and also avoid creating a deprecation on the separator block. It was using background colors, but this didn't work for the dotted separator. |
|
Thanks for the added context @cbravobernal 🙇 I couldn't find any filter specific to the Separator block. I could only see the additional processing of style declarations in PHP, which didn't match the JS as it processed the theme.json data instead. From the sounds of it, there wasn't a specific technical reason for that and we should be fine to make that more consistent while relocating the updates to the theme.json layer it relates to i.e. the theme layer for original fix, user layer for the fix I'm working on. The Separator block and its different rendering and styling for different block styles, e.g. Dots, is rather painful. The approach I'm taking in #67269 also avoids the need for deprecations so I believe it's aligned with the intent of the original fixes. |
What?
Fixes #43660 using the first option:
Why?
Defining the separator color in a style variation, using background on theme.json is not working. Most variations are done with text color or border color.
How?
Tweaking the stylesheet creation to add the text color if needed. Skipping if there is already a color set.
Testing Instructions