Update the paragraph block to use the colors support key #21037
Conversation
|
Size Change: -3.57 kB (0%) Total Size: 856 kB
ℹ️ View Unchanged
|
| @@ -1,3 +1,8 @@ | |||
| p { | |||
There was a problem hiding this comment.
Are you sure it’s safe to set those values for all paragraphs including front-end as part of the block definition?
There was a problem hiding this comment.
That's a good question and one we can only address by trying some themes. It does require a devnote but I think it's for the best.
There was a problem hiding this comment.
My second thought is that it's probably fine if you consider that with the full site editing, you should create the whole page using only blocks. If someone is using a hardcoded paragraph or heading, it means they might reconsider using nested blocks.
There was a problem hiding this comment.
I think applying these styles widely will be a very major change most current themes are not expecting that p elements have color and background property set by default.
When a theme does .content { color: 'red' } probably the expectation was that the color would be applied to paragraphs inside the content and now that will not be the case.
During previous tries with @q and @nosolosw we used wp-gs class to signal a theme support global styles and so it was safe to add these styles. The problem is that here we want the variables set if the theme does not support global styles as they required for things to work, so I'm not sure of the best path.
There was a problem hiding this comment.
😍
I've got #21021 in my review queue, so I'm not fully up to date on how this works. Seeing this is the result, though, I'm pretty excited about it! As to how this could be rolled out, I see 3 approaches. I'll list them from ideal to less ideal:
1. What you're proposing here.
I like this the best. However, it'll require themes to adapt their code. There's no nice fallback mechanism, according to how the cascade works with CSS properties.
Example of how may it work for a property that is inherited (color):
- the theme sets
body, .content { color: black } - the paragraph block uses the proposal here and core doesn't enqueue any value for
--wp--text-color - because the CSS property is invalid and
coloris an inherited property, its value will be taken from the nearest ancestor
Example of how would it fail for a property that cascades (color):
- the theme sets
p { color: black; } - the paragraph block uses the proposal here and core doesn't enqueue any value for
--wp--text-color - because the CSS property is invalid and
coloris an inherited property, its value will be taken from the nearest ancestor and the theme rule targeting thepis ignored
Example of how would it fail for a property that doesn't cascade (background-color):
- the theme sets
body, .content { background-color: black } - the paragraph block uses the proposal here and core doesn't enqueue any value for
--wp--background-color - because the CSS property is invalid and
background-coloris not inherited, its value will be taken from the user agent
2. Only enqueue the styles containing the CSS variables if theme opts-in.
I've proposed this at #20273 and I'd like to go with this, at a minimum, during the exploration phase.
I don't know how it'd work with the changes at #21021, though. I also wanted to note that, although in 20273 I proposed an opt-in mechanism that is specific to "global styles", it doesn't need to be that way. We could use, for example, the existing editor styles --- I haven't dug into this deeper, but wanted to share anyway.
3. Add a class to scope the styles.
This is the last resort for me. It's comfortable for us but it creates a split API for themes: when they want to style a block they'll have to know the block internals and they'll end up with something like:
.wp-block-quote { ... }
.wp-global-styles.wp-block-quote { ... }
There was a problem hiding this comment.
I think core will always include a value for --wp--text-color right? The idea is that we provide defaults for the variables
I'm not sure I agree with this. The paragraph doesn't need a default color to work?
There was a problem hiding this comment.
Yes, the paragraph does not need a default color. But I'm generally speaking I think we want to pass the idea block developers can use global styles variables and know they will have sensible defaults, right? The variable --wp--text-color may be used in other places besides a paragraph.
If the variables don't have a default, blocks will need to provide their defaults, and we may have two blocks both using --wp--text-color for something so ideally their text color would be the same but as we don't provide defaults both blocks may use their defaults and have different colors. It seems if we don't provide a set of defaults for all the variables, the experience is going to be inconsistent.
There was a problem hiding this comment.
the idea block developers can use global styles variables and know they will have sensible defaults, right?
I don't think so personally. If a block needs a default for something, it should just provide it as a fallback.
There was a problem hiding this comment.
Also for most of these variables, the default will be the browser default anyway as the default need to be unopinionated.
There was a problem hiding this comment.
After some more discussions about this, we're going with the :root p { color: var( --wp--color--text ) to give it more specificity. It is granted that this has the potential to break some theme, themes could fix with several ways including the theme.json config file and it puts us in a better position for FSE and global styles theme compatibility.
We'll have to communicate this on the next plugin release. We'll keep monitoring themes and adapt.
One of the potential solutions we could explore based on that feedback is to apply aa has-styles class whenever a CSS variable is used (color or anything else)
youknowriad
force-pushed
the
update/group-colors
branch
from
99a94f1
to
92c9e3b
Compare
youknowriad
force-pushed
the
update/paragraph-ccolors
branch
from
97fbcbf
to
c890ffd
Compare
youknowriad
force-pushed
the
update/group-colors
branch
from
92ef4bb
to
dfca58c
Compare
youknowriad
force-pushed
the
update/paragraph-ccolors
branch
from
c890ffd
to
37c3572
Compare
youknowriad
added
[Block] Paragraph
|
This would fix #21163. What's blocking this? |
|
We need some testing across themes to check the conflicts with CSS variables. |
|
also fixes #20810 |
|
Anyone to give the ✅here :) |
|
Hold your horses! The Color Settings panel is missing when I tested this PR. Not sure if its just me!
|
|
I can see them just fine, maybe something in your setup? |
|
Okay, double-checked. The Color Settings panel is missing when I cherry-picked your commits on top of the 7.8.0 tag. The Color Settings is intact when I checked out this PR's branch. |
|
Hi @youknowriad, |
Mmm right! potentially we could set the CSS variable in the classes. I'd love if we could generate these styles to avoid telling block authors to update their CSS |
|
Alternatively, if you think we should explore switching to CSS variables for the named colors in a PR. That would work too. |
This solution seems to be more future proof. |
| }, | ||
| }; | ||
|
|
||
| const migrateCustomColors = ( attributes ) => { |
There was a problem hiding this comment.
Would it make sense to share this migration function across our blocks?
There was a problem hiding this comment.
yes, I duplicated for now but yes it's always the same.
youknowriad
removed
the
Needs Dev Note
Fixes #21163.
Based on #21021 to try test the API on different blocks (paragraph for this PR)