Block Supports: Add border color, style and width support #30124
Conversation
aaronrobertshaw
added
[Block] Group
aaronrobertshaw
requested review from
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
|
@nosolosw I've made a start on adding the extra properties to the border block support. It would be great if you could spare a few moments to sanity check the approach taken for the border colors. Thanks in advance! |
|
Size Change: +37.3 kB (+3%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
gziolo
added
the
Global Styles
|
Thanks for the time and effort on the review @grzim. Appreciate it 👍 |
aaronrobertshaw
force-pushed
the
update/border-block-support
branch
from
0f3b345
to
c8071d9
Compare
|
I've rebased this PR and added skipping serialization of border support properties. There have also been a few inline comments added to help clarify the intent of the code. |
There was a problem hiding this comment.
This tests well for me in both the editor and on the frontend, with various permutations of the available settings. I also tested enabling only a subset of the color/style/width/radius options in the theme.json, omitting the color support for example.
LGTM, this would be a great addition 🎉
|
A new border panel has been added to the Global Styles sidebar and the PRs testing instructions updated. This side of the changes could do with closer inspection and testing. |
| ...style, | ||
| border: { | ||
| ...style?.border, | ||
| color: colorObject?.slug ? undefined : value, |
There was a problem hiding this comment.
I'm just testing this PR and, for me, the border color panel doesn't add the border color inline style to the root element. The has-{color}-border-color class is present however.
The result is that the border color doesn't change when I select a new border color.
Strangely, changing the text color modifies the border color, though I can't see why in the computed styles.
If I change this line to color: colorObject?.color || value, it works as I'd expect.
Is there a reason why the style prop should be undefined here in the presence of colorObject?.slug?
Or are there CSS definitions for .has-{color}-border-color classes that I'm not seeing (or something else I'm missing)?
Thanks!
There was a problem hiding this comment.
There's a good chance I missed something here so I'll look into it. Thanks for testing.
Is there a reason why the style prop should be undefined here?
This was undefined so that when using a named color it uses the CSS classes generated for the color from the current palette. If it stored the color value there the inline style overrules it. This could become a problem if the theme changes the color palette or the user switches themes.
There was a problem hiding this comment.
Strangely, changing the text color modifies the border color, though I can't see why in the computed styles.
I have the suspicion that the theme sets the border color to the currentColor which would be determined by the text color. The inline border style works for me when a border style is set though.
There was a problem hiding this comment.
This was undefined so that when using a named color it uses the CSS classes generated for the color from the current palette.
Thanks, that's what I suspected, was just scratching my head as to where those CSS class definitions were being (or should be) generated.
There was a problem hiding this comment.
Thanks for the clarification. Just dropping here to add that I had the TwentyTwentyOne theme activated.
I could see the expected behaviour after switching to TT1 Blocks.
There was a problem hiding this comment.
@nosolosw would you be able to point me in the right direction for getting the color styles generated from the theme.json color palette available for themes such as TwentyTwenty?
I believe the active theme currently has to satisfy this check before the necessary styles are included.
There was a problem hiding this comment.
useEditorFeature( 'color.palette' ) will have the color palette, whether or not the theme has support for theme.json
There was a problem hiding this comment.
The issue I'm trying to solve here is some older themes don't qualify to have the named color CSS classes included. In other words they are missing has-<color>-border-color.
If the TT1 Blocks or TwentyTwenty Blocks themes are active, they get the generated color classes fine. Something older such as the TwentyTwenty (non block based) theme doesn't get these so while the block has the correct CSS class on the frontend the CSS class doesn't exist in the stylesheet.
There was a problem hiding this comment.
Ah, I see. Themes that don't support theme.json won't have the preset classes automatically enqueued for them, they add them themselves. At some point in the past, we experimented with enqueuing the classes for these themes as well but heard reports that this wasn't expected, so we stopped doing it.
What I see is that named border-color classes are properly attached to the block (for any theme), and that's the only thing relevant for the hook. In terms of the global stylesheet, I also see them generated and enqueued in the embedded stylesheet global-styles-inline-css for themes with theme.json support, so everything is working as expected.
One thing we should do is to add this as a dev note saying: there's this new property so themes need to enqueue the classes for the border-colors as well. Only if this feature becomes stable for 5.8. I'll add the label so we can find them later.
Also ccing a few theme people FYI (re: has-<color>-border-color classes) ― @carolinan @aristath @scruffian @MaggieCabrera
There was a problem hiding this comment.
Thanks for the explanation. That helps my understanding 👍
Passing these props prevents the need for the ColorGradientControl to retrieve the colors a second time.
|
Going to add the dev note label as per this conversation https://github.com/WordPress/gutenberg/pull/30124/files#r614248792 |
oandregal
added
the
Needs Dev Note
There was a problem hiding this comment.
I've left a note that I'd like addressed at https://github.com/WordPress/gutenberg/pull/30124/files#r614149424 Other than that it's working nicely and the code is ready.
Great work here. Thanks for your patience and persistence!
I've fixed up the support check code as requested. It should be pretty close now 🚢
Team effort! I appreciate all the time you've spent reviewing this and other PRs 🙇 |
|
Added another PR for updating the |
|
We forgot to register the border color preset in the client as well, so the preset classes are not being generated in the site editor. Fix for this at #31217 |
youknowriad
removed
the
Needs Dev Note
|
I remove the dev note label because it's still experimental we should add the label to the PR that make this stable. |
Fixes: #29616
Description
This PR extends the border related block support to include color, style and width options.
These border options are disabled by default.
How has this been tested?
Manually with a couple of small updates to existing unit tests.
Testing Instructions:
Unit Tests:
npm run test-unit:watch packages/block-editor/src/hooks/test/style.jswp-env run phpunit "phpunit -c /var/www/html/wp-content/plugins/gutenberg/phpunit.xml.dist /var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-theme-json-test.php"Screenshots
Types of changes
New feature
Next steps
Checklist: