Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce a support key for support global style colors in blocks #21021
Introduce a support key for support global style colors in blocks #21021
Changes from 16 commits
dded0b1
6bedd0f
a7805db
f9b812b
37e578a
34f0584
d25356b
a9cc665
8812560
45a84dd
076224b
59eb590
1468aaa
92c9e3b
72d9905
583f1b2
dfca58c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I have an example of CSS for named colors using variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in theme.json a theme may provide something like:
This would generate the following vars during the compilation:
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it’s not that different from what we had before but if you would see a way to extract a hook that operates on DOM and put it in its own file it might make it easier to replicate this behavior in React Native. Well, the best case scenario is it becomes no-op there 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's similar and I agree, it seems like it could be its own hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like here we should filter out keys that were set to
undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are cases that are not background color or text color, e.g: blocks may allow choosing border color and hoover colors:
Other blocks have multiple text areas (multiple rich text elements) and have multiple color pickers for each text area plus color pickers for the separators:
For these cases, the plan is to expand this API in the future? Or should they use the UI components plus useColors or the UI Components plus withColors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, probably a custom implementation using the reusable components. I'm still not sure about withColors or useColors, I'm not convinced by both. We'll see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't worry about it too much at the moment. It will naturally evolve together with the development of other design-related features lead by @ItsJonQ. At some point, I expect that Global Styles and the block editor features API will intersect with this logic. This PR provides the best possible defaults that make the most sense as confirmed in the follow-up PRs. Once we introduce more color-related features like borders, hover and focus states, we could control them with block editor features per site or per block.
It raises the question of whether they could be refactored to use nested blocks that would solve the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had trouble understanding the API of the PanelColorSettings. I believe it should be documented properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we going to support configurable labels? E.g: color property may not be a text color in some contexts it may be an icon color for example, in the cover a background color is not a background it is "Overlay".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not a text color or a background color, the CSS variable names and global styles config doesn't make sense, so this hook shouldn't be used in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are assuming a block will always have a text and a background color, currently, the heading block just has a text color. Should we still support these use cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, It's easy enough (commented about it on #21039 ), I'm trying to see how far we can go without any config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In cases that support gradient backgrounds, the UI for gradients should appear in the same panel as colors, should these hooks also handle gradients?
It would just require that we check for gradient support. Then we could unconditionally use PanelColorGradientSettings instead of PanelColorSettings and if gradients are supported we would pass different settings to the component.
I guess it could work well for the button to cover the logic is a little bit more complex as when gradients are added together with an image we need to insert a new dom element and the gradient there so I guess for cover we will always need some custom code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I wonder if we should auto-support gradients for all blocks that support background colors but regardless, I believe this hook should support gradients in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About whether we should auto-add the sub-element when gradients are used, ideally no but we might have to, the goal is to always be generic though as we need to be able to support consistent features across blocks way more quickly than what we do today. If we sacrifice some flexibility sometimes, it's fine.