List block: add color controls #21387
Conversation
ZebulanStanphill
added
[Type] Enhancement
|
Size Change: +57 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
from
45ad91f to
40e3357
Compare
|
I need some help with the styles. The Twenty Twenty styles override all of them. |
ZebulanStanphill
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
from
40e3357 to
4f7aebb
Compare
|
I rebased and revised this PR after the changes in #21428. I tested in all the default WordPress themes. The good news is that the styles worked on the front-end in all of them. The bad news is that there were always issues in the editor: In Twenty Twenty, background padding is not applied, and when a background color is set, the theme styles override some (but not all) of the text color options. In Twenty Nineteen, only custom (non-palette) background/text colors work. Background padding is not applied. In Twenty Seventeen, all styles except the background padding are applied properly. Twenty Sixteen, Twenty Fifteen, Twenty Fourteen, Twenty Thirteen, Twenty Twelve, and Twenty Eleven all behave like Twenty Nineteen. Twenty Ten was the worst of all. Only custom background colors were applied. Even custom text colors didn't work. |
|
For fun, I also tested the Classic theme (which appears to have no editor style support whatsoever). It behaved like Twenty Seventeen. It looks like the background padding style issue in the editor is caused by this style in the default editor stylesheet. |
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
from
4f7aebb to
7f69226
Compare
| @@ -0,0 +1,4 @@ | |||
| ol.has-background, | |||
| ul.has-background { | |||
| padding: $block-bg-padding--v $block-bg-padding--h; | |||
There was a problem hiding this comment.
This padding is being overridden by the default editor styles. You might want to make the selector .block-editor-block-list__block.has-background instead (that should also work for both types of lists)
There was a problem hiding this comment.
block-editor-block-list__block is a class applied to all blocks in the editor, so I don't think I want to use that selector. (Also, that class is only applied in the editor.)
The default editor style rule (which gets loaded as .editor-styles-wrapper ol, .editor-styles-wrapper ul) has equal specificity as ol.has-background, ul.has-background. The only reason why the default editor styles take precedence is because they are loaded after the block styles. That sounds like a bug to me.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
I've created #24011 to track this bug.
There was a problem hiding this comment.
That issue is now fixed. I've created #30094 to remove the now-unnecessary editor styles.
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
2 times, most recently
from
d95284b to
cfe8684
Compare
| @@ -0,0 +1,4 @@ | |||
| ol.has-background, | |||
| ul.has-background { | |||
| padding: $block-bg-padding--v $block-bg-padding--h; | |||
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
from
cfe8684 to
c470307
Compare
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
from
c470307 to
eaa9346
Compare
|
After switching to a doubled CSS class to account for the CSS override issue described here, background padding is correctly applied in Twenty Twenty and Twenty Nineteen. In Twenty Seventeen, the selector to apply the background padding still isn't strong enough to take precedence. It's being overridden by a style rule targeting (On a side note, the margin on List blocks seems to be broken in Twenty Seventeen in the editor, and it appears to have nothing to do with this PR. It looks like there's a regression caused by a change in |
kjellr
added
the
[Feature] Themes
ZebulanStanphill
changed the title
|
Let's add gradients and ship this. |
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
from
eaa9346 to
1a1a205
Compare
|
I'm still not sure yet about the solution for the specificity issues here but I'd like to find a way to communicate the changes properly to theme authors. Since the I'd love eyes from @kjellr @jasmussen about the path forward here. |
|
Keep in mind that the styles always work on the front-end with all themes. It's only the editor styles that have specificity issues. I had hoped that something like #21102 would have been merged by now, but it looks like that is still not ready, and I doubt it will make it into WP 5.5. |
|
Thanks for the PR, you've been making lots of great stuff, Zeb, thank you. Before I comment on specificity issues, I wanted to circle back to this one:
One of the changes we have to make in the not too distant future, is to refactor both the list block and the quote block to hold nested blocks. The structure should ideally be List > Paragraph and Quote > Paragraph, because in HTML you could easily have an image (that isn't inline) in a list, or a nested quote inside a quote. We've known this for a while, but have held off because the select-parent situation hasn't been good enough. However, it's really getting there, and perhaps with some tweaks to the "parent absorbs child block toolbar" which @MichaelArestad has been thinking about, we can move forward. Not in time for 5.5, but maybe shortly after. When that happens, and it does have to happen at some point, will the change in this PR become vestigial? Or will it change to be more like how color settings inside the Group block is? |
|
For me, the use of nested blocks don't have any impact on this PR as it applies those styles to the wrapper which exists in both cases. |
If and when that happens, it is likely going to happen in a way where if a plugin is detected which relies on there not being an iframe, we don't iframe the canvas. We can probably accept an amount of partial brokenness after a transition period. I feel the same way about specificity here, that more important than anything, we feel the approach is the right approach, and then themes can update. It can get painful at moments, but in the end we are making these improvements with low specificity, including the iframe efforts, to benefit themers. |
I haven't tested, but from what I read above, it sounds like it's likely related to the issue that #22356 is trying to fix. In general, the color palettes aren't working in most of these themes right now.
In general, I think the approach here seems fine. I don't totally love the double selector specificity used in the editor, since it may be difficult or non-intuitive for theme authors to override that. Does it work without that in most other themes you've tested with? If so, it might be worth just patching Twenty Nineteen and Twenty Twenty if those are the problematic ones. In terms of communication, we can include this in the Gutenberg + Themes roundup to start. That might help highlight it alongside the usual release notes. |
|
Parent blocks should generally have color controls even if their children also have them (the Group block has background and text color controls), so whether the List block starts using nested blocks or not is irrelevant, I think.
@kjellr The double selector for padding is needed to override core Gutenberg styles, not theme styles. The problem is that a certain default editor stylesheet is loaded after the default individual block editor styles, which I'm pretty sure is a bug. |
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
from
1a1a205 to
d8708af
Compare
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
2 times, most recently
from
38fbc27 to
aac320a
Compare
ZebulanStanphill
removed
the
Needs Design Feedback
|
It looks like #22356 fixed the color palette issues. So this is mostly ready to merge. I just have one question: Should I merge this with the workaround for the default editor styles being loaded later? Or should I merge it without that workaround and hope the issue gets properly fixed later on? |
Good question that probably needs more than my opionion. And I'd personally prefer no workaround, but if workaround be sure to document it with a code comment and a note to remove it if a particular ticket gets addressed. |
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
from
e4d4367 to
fac145c
Compare
ZebulanStanphill
force-pushed
the
add/list-block-color-controls
branch
from
fac145c to
3a943ea
Compare
|
Rebased and improved the inline comment note to link to the related issue on GitHub. |
|
@youknowriad Just to be clear, are you okay with this being merged with the workaround for #24011 included? |
|
yes, I'm personally fine with that. This is not the only place where such issue can happen and the value provided by this PR is important enough that we can live with the small workaround while we figure out a better way to load styles. |
Description
Adds color controls to the List block. If the Paragraph and Headings blocks are going to have color controls, then so should this one. Closes #9016.
Checklist: