Add gradient and color support in Quote #24968
Conversation
ntsekouras
added
[Block] Quote
ntsekouras
requested review from
ajitbohra,
Soean and
talldan
as code owners
|
Size Change: +4.92 kB (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
|
I've taken a look at this and it looks like we need to remove the failing test. The reason it fails is because:
A related question is: is it ok adding an attribute with the same name than a deprecated one? In this case, it's fine:
|
| @@ -29,4 +29,8 @@ | |||
| &.is-large { | |||
| border: none; | |||
| } | |||
|
|
|||
| .has-background & { | |||
There was a problem hiding this comment.
I saw that we use this padding in other blocks that have background color as well, but we add it in the style.css stylesheet instead of the theme.css. Not sure what's best, perhaps @jasmussen would know?
There was a problem hiding this comment.
Good question.
Thje theme.scss file requires opt-in, style.scss is "baseline styles". Which other blocks are we using this same padding in?
Generally my instinct would be that this remains a bit of an opinionated style, and that the better place for it is in theme.scss so themes can decide themselves exactly how much padding, if any, they want to add to blocks that have background colors. But I'm not strongly of that opinion.
Is there something else we can do? Can Global Styles absorb some of this "if background then add padding" functionality?
There was a problem hiding this comment.
Which other blocks are we using this same padding in?
Blocks that use this hook (color). As an example p.has-background and most of them don't have a theme.scss, they have this rule in styles.scss.
Maybe I should add it to styles.scss like the other ones, to be more consistent.
Can Global Styles absorb some of this "if background then add padding" functionality?
@nosolosw @jorgefilipecosta any thoughts?
There was a problem hiding this comment.
I searched for $block-bg-padding and/or .has-background and this is what I found:
- use styles.scss: columns, heading, list, paragraph.
- use theme.scss: group, separator.
Not sure if there will be more. I don't have a preference and arguments can be made for both options. I trust Joen's instinct, perhaps theme.scss is less disruptive.
Another point for theme.scss is the question about global styles. Riad mentioned that we could look at using support for theme.json to do some other things (like a simpler block markup, cleaning up some styles, etc). I'd leave that out of this PR for now, but just mentioning that's another point for theme.scss (it can be more easily migrated to global styles).
There was a problem hiding this comment.
I'm personally not sure if the separation between theme.scss and style.scss was ever a successful thing.
There was a problem hiding this comment.
I would agree with that, but I do think the goal of the separation was noble, and it would be nice if global styles could help there.
Two examples come to mind, when we started this, the figcaption element was rarely styled and looked out of place with his size and margins, so that was given a size and margin. And the quote had a left border.
For a theme that does style those two things, having to undo or override those styles is annoying. Therefore the separation.
If global styles could provide these came opinionated designs but allow themes to replace them (instead of overriding or unstyling), that could be a better solution.
|
I see this:
That is, cool stuff, this is nice! However, a few things. The font size doesn't work in my theme because in my editor style, I set the font size of the What would we do with a group block? Would we ever enable font size controls there? We'd have the same inheritance problem, and we know the quote block will eventually support innerblocks, at which point you can just set the font size on the paragraph block inside. In other words, should we decide on how to handle this before we add font size and line height support? The gradient is cool. Don't mind the swatches breaking onto a new line, I know why that is and will fix that separately. |
|
Nik, this is great work! Technically, this is ready. A different question that I have is: to what extent we should do this. To understand this better, I tested this PR with the last 3 default themes: TwentySeventeen, TwentyNineteen, TwentyTwenty. These are the results:
Not very promising. I think part of the issue is that the block represents two entities (the text + the cite) and our current style system doesn't accommodate this use case very well yet. 🤔 In thinking about how to continue I reckon I don't have a strong answer. Sharing my thoughts in case they're useful:
What do you think? |
|
Hey @nosolosw ! Your feedback is really helpful!
All your points make sense and I'll change the code to add only the Regarding the About the |
|
I think this block was meant to be refactored to use inner blocks at some point, it may solve some of the theme incompatibilities? |
That wouldn't solve this problem:
So even if the blockquote has a specificed font size or margin, so long as any blocks inside also have that, it won't affect the children. |
ntsekouras
changed the title
|
If the block is refactored to use inner blocks, the inner blocks (paragraph, cite) already have design tools and the value will be attached to them (paragraph has font-size and colors, etc). In this case, we could make it so the wrapper (blockquote) only has attached the properties that are relevant to it (background color) but not the other ones that will conflict (font-size, etc). How does this look? |
I may be missing some nuance to your question, but I'm imagining the following blocks: ☝️ The block list above shows a paragraph, a quote, and a paragraph. The Quote has 3 nested innerblocks, one of which is an image. Paragraphs can have text colors changed, as they always have been able to. The Quote block, just like a group block, can change colors that are inherited. But it makes less sense for it to be able to set font size and lineheight, as those are best customized on the Paragraphs themselves. Does that make sense? |
I'm not sure if you're suggesting refactoring I am not very familiar with the nuances of global styles so it's not clear to me about what this PR's direction should be with both comments above ( @nosolosw , @jasmussen ). Would be adding only the |
Not specifically but if this PR is blocked we can consider trying that on its own separately before moving back to this one. |
|
My point was that using inner blocks would fix the issues with themes because the inner blocks already have the design tools, so we don't have to add them to the quote itself (where they don't work well). Note that this is how the style system works, it's not exactly a Global Styles thing. I guess the next steps for this block could be: 1) converting it to use inner blocks and 2) decide whether adding colors (background/text) to the quote makes sense. I don't have a strong opinion on what should be done first. I perhaps suggest doing the conversion first because the text color doesn't work for cite. An alternative would be to implement #24927 so we can decide whether a block has text/background color independently and then add only background color for this block. |
|
Also worth noting that which ever path forward we choose, it should assume that quote and list get nesting at some point, and not optimize too much for how they work today. |
|
Based on all the above comments I think it's better to close this PR and prioritize to refactor Thank you all for your input and feedback! |
Description
This PR adds gradient, color, line height and font size support in
Quoteblock and is part of #22700.This PR surfaced a problem with a broken Quote deprecation, that is being removed here.
How has this been tested?
I've tested with the default themes and I could see that many of them override some of these options. For example a theme could override explicitly the
colorof the Quote and otherspadding. By looking over other related PRs I observed that this is the case for the other blocks as well, so I believe this is okay as themes should have thelast sayin some styles.