Pullquote Block: Add padding and margin support #45731
Conversation
t-hamano
added
[Block] Pullquote
t-hamano
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
|
|
Size Change: +81 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for putting this PR together @t-hamano 👍
It generally tests well for me. However, there are a few issues that we need to address.
✅ Padding support works
✅ Margin support works in non-TT3 themes tested
❓ Blockquote element margin reset
Resetting the blockquote margin looks like it would be a breaking change for existing pullquote blocks. It also doesn't remove the top margin for the blockquote element's immediate child p element.
See screenshot of top margin on `p` not being removed
Off the top of my head, I think our best option might just be to set a default padding on the pullquote block to replace the margin removed from the blockquote.
This should be easily overridden by the block support you've added, not break existing blocks' layout, and avoid the need for us to write a block deprecation or add to core's theme.json. It would still require fixing up the top margin of the blockquote's inner p element though.
What do you think?
|
Thank you for the review, @aaronrobertshaw !
Do you mean to add one padding as follows? .wp-block-pullquote {
// padding: 3em 0;
padding: 4em 0;
}
This is a difficult problem. I have noticed that if I change the blockquote margins to zero, the content is no longer centered at the top and bottom of the block. This is because the Screenshot
I have considered the following two approaches, what do you think? Don't change
|
|
@t-hamano would it be possible to make the Pullquote block have a padding value in design tools as a default, IE:
That would maintain backwards compatibility while giving users control to customise. |
|
Thanks for iterating on this PR @t-hamano 👍
Yes, adjusting the padding to accommodate the margin that is removed in this PR allows us to keep the block visually the same.
I raised this in my review; however, I didn't get the chance to test it then. As you noted, it works fine.
Removing the top margin for the inner Here's a diff that works for me and keeps the block's spacing and dimensions the same as on trunk. With these changes, we avoid the clunky inner margins, allow padding overrides and have the ability to remove padding entirely if so desired etc. Example diff fixing inner spacingdiff --git a/packages/block-library/src/pullquote/block.json b/packages/block-library/src/pullquote/block.json
index 2cad2d2c03..e669955a4a 100644
--- a/packages/block-library/src/pullquote/block.json
+++ b/packages/block-library/src/pullquote/block.json
@@ -70,6 +70,14 @@
"typography": {
"fontSize": "1.5em",
"lineHeight": "1.6"
+ },
+ "spacing": {
+ "padding": {
+ "top": "4em",
+ "right": "0",
+ "bottom": "4em",
+ "left": "0"
+ }
}
}
},
diff --git a/packages/block-library/src/pullquote/style.scss b/packages/block-library/src/pullquote/style.scss
index 3100f595bf..f64e831a68 100644
--- a/packages/block-library/src/pullquote/style.scss
+++ b/packages/block-library/src/pullquote/style.scss
@@ -1,6 +1,5 @@
.wp-block-pullquote {
margin: 0 0 1em 0;
- padding: 3em 0;
text-align: center; // Default text-alignment where the `textAlign` attribute value isn't specified.
overflow-wrap: break-word; // Break long strings of text without spaces so they don't overflow the block.
box-sizing: border-box;
@@ -15,6 +14,14 @@
margin: 0;
}
+ p {
+ margin-top: 0;
+
+ &:last-child {
+ margin-bottom: 0;
+ }
+ }
+
&.alignleft,
&.alignright {
max-width: $content-width * 0.5;
If I've missed something, let me know.
@jameskoster we can define the default block padding via the block.json's Screenshot of Global Styles > Blocks > Pullquote > Layout
It is worth noting, though, that it will not show in the padding controls in the block editor. Unfortunately, we don't have access to the merged global styles there yet as we've discussed on other PRs. The PR to keep an eye on for that functionality is #34178. |
|
Thank you for your detailed suggestions, @aaronrobertshaw !
This may have been my unnecessary concern 😅 I have applied your suggestion and it seems to work generally well with some default themes. On the left is Twenty Twenty ThreeThe default margins for block quotes have been reset, so the left and right spaces are no longer present.
Twenty Twenty TwoThe default margins for block quotes have been reset, so the left and right spaces are no longer present.
Twenty Twenty OneNo change because the theme's padding and margins take precedence.
Twenty TwentyNo change because the theme's padding and margins take precedence.
For the classic themes Twenty Twenty One and Twenty Twenty, this change would not affect them because the themes have padding and margins. For the block themes Twenty Twenty Three and Twenty Twenty Two, the default margins on the left and right side of the |
There was a problem hiding this comment.
Thanks for iterating on this one @t-hamano and apologies for the slow reply.
The addition of padding and margin support is generally testing well for me.
For the block themes Twenty Twenty Three and Twenty Twenty Two, the default margins on the left and right side of the blockquote will disappear, but since we have the freedom to change the padding of the block support, we do not see a problem.
I'm not sure what the best option is here. As you describe, removing the default left/right margins on the blockquote will make that quote text etc go right up to the very edge of the block. This might be undesirable if the block has a background color.
The alternative might be to add a default left/right padding but depending on theme styles etc that might introduce a touch extra space.
Perhaps we should get some design feedback or themes team input here before proceeding?
There have also been some cases lately where we've put PRs adopting spacing supports on ice as layout support breaks margin support under some conditions, e.g. nesting within a Row block.
|
Thanks for the feedback, @aaronrobertshaw!
Yes, I think you are right about this. @WordPress/gutenberg-design Could anyone give us their opinion on this issue? 🙏 The key points of this PR are as follows:
|
|
Great PR, thank you. Also a tricky one. Pullquote is a bit of an opinionated block, and in being out there, naturally has some back compat concerns to think about. In general I don't personally have a strong opinion about the left/right padding by default. From what I can telll, the padding doesn't exist today, but this PR adds it? Weirdly I'm seeing what appears to be the opposite effect testing in TT3. Before: After:
On a separate note, when migrating towards theme.json configurable values, in general it's nice to see fewer values in style.scss as a result, as those are always highly opinionated. In this case, the opposite happens, with more values. It may be fine since pullquote is by design an opinionated block, but nevertheless — a goal ideally is to make those rigid hardcoded values into user configurable ones. So designwise, my opinion is this: the visual changes are fine, and the more we can make user configurable, the better. But ideally this comes at the reduction of hardcoded values. But technically, I think the more tricky conversation here is on back compat. I'll refer to others on how to best handle that. Can the above thoughts help inspire a way to thread the needle? |
|
@jasmussen
The reason there is space on the left and right in trunk is that the Blockquote has 1em margins on the left and right in the trunk post editor
Blockquote has 40px margins on the left and right in the trunk site editor
In this PR, the blockquote margins are all changed to zero, and as a result the left and right spaces disappear. |
|
In my search for a solution, I discovered a problem in the classic theme. If the theme doesn't apply any styles regarding spaces to the Pullquote block, this PR causes the top and bottom spaces to completely disappear. Trunk
This PR
I expect the reason for this problem is that If so, what about updating the CSS as before, without using using Suggested diffdiff --git a/packages/block-library/src/pullquote/block.json b/packages/block-library/src/pullquote/block.json
index e669955a4a..2cad2d2c03 100644
--- a/packages/block-library/src/pullquote/block.json
+++ b/packages/block-library/src/pullquote/block.json
@@ -70,14 +70,6 @@
"typography": {
"fontSize": "1.5em",
"lineHeight": "1.6"
- },
- "spacing": {
- "padding": {
- "top": "4em",
- "right": "0",
- "bottom": "4em",
- "left": "0"
- }
}
}
},
diff --git a/packages/block-library/src/pullquote/style.scss b/packages/block-library/src/pullquote/style.scss
index f64e831a68..fa3c741bff 100644
--- a/packages/block-library/src/pullquote/style.scss
+++ b/packages/block-library/src/pullquote/style.scss
@@ -1,5 +1,6 @@
.wp-block-pullquote {
margin: 0 0 1em 0;
+ padding: 4em 1em;
text-align: center; // Default text-alignment where the `textAlign` attribute value isn't specified.
overflow-wrap: break-word; // Break long strings of text without spaces so they don't overflow the block.
box-sizing: border-box; |
Oh that's useful, thank you! Looks like those 1em margins come from common.css, which means it's wp-admin CSS bleed, i.e. not something we should design for. What are the margins on the frontend? Whatever that is, should probably inform the new default. Nice! |
That's my understanding as well although it looks like there is talk about making sure these block styles are loaded for classic themes like presets and CSS vars are. There is also an open issue tracking the efforts to absorb block CSS into the global styles mechanism. However, looking at that issue it appears that in several cases, the I'm really not sure what direction we are supposed to be moving in here. Perhaps @oandregal and @scruffian might be able to share their wisdom? |
|
Sorry for the delay in replying. I checked #43981 and the issue of
It might be better to hold off on the case of adding Or should we not use |
Since the margins are coming from |
I tested this PR and this is what I see:
It sounds like everything is working as expected on that front? |
|
@oandregal
Many default themes have padding applied to this block on the editor. Therefore, it does not matter if Is this the intended behavior? |
|
@oandregal |
There was a problem hiding this comment.
@t-hamano what's the plan for this PR?
I believe the issue with __experimentalStyle hasn't been resolved yet (#46818). Can we get away with removing the spacing changes to __experimentalStyle and adding the updated padding back to the block's stylesheet?
Giving that a quick test using emptytheme seemed ok. It still doesn't seem to be a perfect fit though.
|
@aaronrobertshaw Sorry for replying so late.
I think this approach makes sense. In the next few days, I'll be testing with different themes, including Emptytheme within several days, and will update this PR if necessary. |
There was a problem hiding this comment.
Thanks for chipping away on this one @t-hamano 👍
I've given this another test and it's looking good.
🚢
|
@aaronrobertshaw Thanks for the review! I tested it again and it seems to be working as expected with the main themes, so I would like to merge it. |
|
On a block theme with no custom spacing and no custom settings or styles on the pullquote block, the padding change is a visible change when the block is aligned. 6.4.3: 6.5 / Gutenberg trunk I know it looks a bit strange because the aligned blocks do not clear the alignment, but that is a separate issue. |
|
The visual change is due to overriding the browser's default margin applied to the blockquote element inside the block with zero. It is possible to add about 1em of padding to the left and right of the block in place of the disappeared margin, but I think it would be difficult to maintain the appearance of all themes whether we add it or not. This is because the theme may already add padding to the left and right sides of the block, or change the margin of the blockquote element. If we add padding to the left and right:
If we don't add padding to the left and right:
I personally think the visual changes made by this PR are acceptable, what do you think? |
|
Regarding this PR, it might be better to have a Dev Note. I suggest notes like below as part of the miscellaneous dev note, what do you think? Pullquote block now supports padding and margin. At the same time, the browser default margin applied to the |
|
Yeah I think that is the best alternative. |
carolinan
added
the
has dev note
What?
Add padding and margin support to the Pullquote block.
Fix #44129 at the same time.
Why?
To create consistency across blocks.
How?
Added the relevant block supports in block.json. Furthermore, now that the margins can be controlled, I think the default margins for a
blockquoteinside afigureshould be reset.Testing Instructions
AS for
marginsupport, this can't be tested on Twenty Twenty Three. This is because the top / bottom margins are defined asimportantin theme.json. If this PR is merged, theme.json of Twenty Twenty Three might need to be updated.Screenshots or screencast
4e75a98b376237bbee419ed3e8e060b7.mp4