Introduce a support key for support global style colors in blocks #21021
Conversation
youknowriad
added
[Feature] Block API
youknowriad
requested review from
mtias,
mcsf,
oandregal,
aduth,
ItsJonQ and
jorgefilipecosta
|
Size Change: +912 B (0%) Total Size: 859 kB
ℹ️ View Unchanged
|
| <ColorPanel | ||
| key="colors" | ||
| clientId={ props.clientId } | ||
| colorSettings={ [ |
There was a problem hiding this comment.
I had trouble understanding the API of the PanelColorSettings. I believe it should be documented properly.
|
|
||
| /** | ||
| * Filters registered block settings, extending attributes to include | ||
| * `backgroundColor` and `textColor` attribute. |
There was a problem hiding this comment.
Should we try to have a "style" attribute that blocks and global styles can use to store global styles related information, instead of using the hook to register a new attribute?
There was a problem hiding this comment.
That's already the case in this PR. These extra attributes are needed for the color palette classNames unless there's a way to define these with global styles that I'm not aware of?
There was a problem hiding this comment.
I think @mtias suggested that instead of using classes for predefined colors we should still use CSS vars anyway but the CSS vars would reference another vars.
There was a problem hiding this comment.
I guess we may have a way to store things in style that indicates another variable is being referenced.
Global styles may have a mechanism where themes structurally set the variables, and we can say colors variables should reference some set, so when we set a color to a value equal to the variable part of a set instead of storing the value we reference the variable.
There was a problem hiding this comment.
Can I have an example of CSS for named colors using variables?
There was a problem hiding this comment.
So in theme.json a theme may provide something like:
colors: [
{ name: "Red", slug: "red", color: "#f00" },
{ name: "Blue", slug: "blue", color: "#00f" },
]
This would generate the following vars during the compilation:
--wp--colors--red: #f00;
--wp--colors--blue: #00f;
When storing a color attribute if the value is equal to one of the color variables instead of storing the value we would store a reference to the variable: --wp--background-color: var(--wp--colors--red);
In the style attribute, we may use the var directly {"style":{"backgroundColor":"var(--wp--colors--red)"}} or we may use a special reference mechanism syntax {"style":{"backgroundColor":"ref:red"}}
This solution has some disadvantages when compared with class usage:
- A theme can not automatically apply a text color when a specific background color is used, and a text color is not explicitly selected.
- A theme can not apply a hover color depending on the text color.
- A theme can not define the colors of links in a block, depending on the text color selected.
There was a problem hiding this comment.
This is an interesting proposal and I can be onboard with that. Though, that's a bigger separate project. If we do this, we need to do it across blocks by measuring the pros and cons and communicating the changes properly.
For this PR, I think we can keep the existing behavior and it can still be adapted in the future if we use CSS variables for palettes too.
There was a problem hiding this comment.
If we would use CSS variables for registered theme colors, we could avoid having all those class names stored in HTML content generated by the save method altogether.
I think it feels like the very next step to explore. In theory, it could also open doors for having only one attribute used per color type as you could use CSS syntax for values.
Co-Authored-By: Zebulan Stanphill <zebulanstanphill@protonmail.com>
youknowriad
force-pushed
the
update/group-colors
branch
from
99a94f1
to
92c9e3b
Compare
|
@aduth Good thoughts. I think the current implementation in this PR has several benefits that can't be achieved using inline styles:
|
|
I think this is ready to land as a v1. It's experimental for now, which means we can still decide to change/revert later if needed and we'd have a full plugin release cycle before the next release. Anyone willing to ✅ |
youknowriad
added
the
Needs Dev Note
|
The logic that set
In my testing, I figured out it gets added when changing the background color. Other than that it works perfectly fine and it's ready to go. Great work Riad! 👏 As discussed, we should verify how it plays in action and based on that take the next steps. |
| @@ -29,8 +29,10 @@ export const settings = { | |||
| ], | |||
| example: { | |||
| attributes: { | |||
| customBackgroundColor: '#ffffff', | |||
| customTextColor: '#000000', | |||
| style: { | |||
There was a problem hiding this comment.
Do you know why this example differs from the actual usage?:
There was a problem hiding this comment.
Outdated example :)
| import { getCSSVariables } from '../style'; | ||
|
|
||
| describe( 'getCSSVariables', () => { | ||
| it( 'should return an empty object when called with undefined', () => { |
There was a problem hiding this comment.
Should there be a test that handle falsy values:
it( 'should omit CSS variables when the provided value is falsy', () => {
expect( getCSSVariables( { color: undefined } ) ).toEqual( {} );
} );
| const attributeName = name + 'Color'; | ||
| const newStyle = { | ||
| ...style, | ||
| color: { |
There was a problem hiding this comment.
It looks like here we should filter out keys that were set to undefined
There was a problem hiding this comment.
It's not easy since it's nested and we should filter out the ones that are not coming from this particular hook.
There was a problem hiding this comment.
It isn't the end of the world if those values are stored as empty, it's just nice optimization to have in place as it is technically not necessary.
youknowriad
force-pushed
the
update/group-colors
branch
from
8130590
to
92ef4bb
Compare
youknowriad
force-pushed
the
update/group-colors
branch
from
92ef4bb
to
dfca58c
Compare
|
Just adding that this should be included in the dev note #21428 (comment) |
youknowriad
removed
the
Needs Dev Note
Yes I know I know, I've been an opponent of "support" keys for blocks for a long time but we can change our minds right? The simplicity they allow for block authors is IMO good, especially for generic features that should be consistent across blocks.
One of these consistent features could be "colors" and it would include things like:
1- Applying a background color (custom or palette)
2- Applying a text color (custom or palette)
3- Contrast checking
4- Applying gradients (custom or palette) as backgrounds
Some very small changes could be possible between blocks, like enabling/disabling gradients, backgrounds, choosing whether to put the controls in the inspector or toolbar, but ultimately these could be provided by the support key too.
In addition to the factorization benefits, this PR is also based on some ideas:
5- Adding support for colors also means adding support for global styles colors (configurable from theme.json files)
6- Support custom colors is not different from supporting say lineHeight, customFontSize... All these options could be considered global styles support and persisted similarly in a unique
styleattribute.The current PR solves 1, 2, 3, 5, 6 and applies a refactor to the Group block to make use of this new support flag.
I also know this makes
useColorskind of redundant but since it's experimental, that's fine and if we find value in this approach we can decide to go with one or the other.