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 👍
talldan
added
the
Backport to WP 6.7 Beta/RC
|
I came across this PR while researching issue #41194 regarding the same separator block color. One concern I have is that the separator block supports gradients. From testing this PR, it appears that gradient colors are not reflected correctly. However, I know that few people use gradients for separators, and I can't imagine how they would be colored in dot style 😅 |
I don't know if we should remove that support. Otherwise, the separator block needs a review, and, in order to work with background and gradients it should have 1px height (or height attribute) and also would be cool to add border supports. After adding that, the next step would be to work on migrations-deprecations. But this solution seems out of the scope for 6.1 😢 |
Thank you, that makes sense 👍 |
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 <mmczaplinski@gmail.com> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
* 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 <mmczaplinski@gmail.com> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
Mamaduka
removed
the
Backport to WP Minor Release
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