Columns: Add padding support #35701
Columns: Add padding support #35701
Conversation
stacimc
added
[Status] In Progress
|
Thanks for digging into this one @stacimc!
It looks like there was a similar issue with Group block. The solution appeared to be to move the |
|
Size Change: +2.32 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is testing nicely for me now in blocks-based themes @stacimc!
It's very cool being able to set consistent padding and block spacing with a background color set 👍
✅ Padding is available but not one of the default controls
✅ Setting a padding value by theme.json works correctly
✅ Setting a padding value by Global Styles works correctly
✅ Columns block-level padding doesn't affect padding of inner Column blocks nor the spacing between blocks
✅ Different combinations of background colors work as expected, and the fallback padding when a background color is present takes least priority (it is applied, but gets overridden by any global styles or styles set at the block level)
❓ I found one issue with the move to theme.scss (sorry for leading us astray, there!) — it turns out that moving to theme.scss results in a regression for themes that haven't explicitly opted in to the block styles. I left a comment, but I think if we move the rule back to style.scss, it should be fine.
Once the rule is moved back to style.scss, I think this PR will be good to land!
There was a problem hiding this comment.
Thanks @stacimc! This is testing well for me now, no regressions in TwentyTwenty and TwentyTwentyOne classic themes, and the padding still works correctly in TT1-Blocks (defined in theme.json, in global styles, at the block level, and if none of those are set, and a background color is selected, the &:where(.has-background) CSS rule takes priority 👍)
LGTM! 🎉
|
Tested well for me. The only thing I noticed is that with tt1-blocks you can't wind the top and bottom padding back to 0 with paragraphs in each column as these have a top and bottom margin: TwentyTwentyOne: TwentyTwentyOne-Blocks: Probably not something we can do anything about though, as the nested column content could have any sort of css that might affect this padding on the outer columns parent - just mentioning in case it sparks any thoughts about ways to override, or other issues that might be encountered with margin settings on inner content. |
I agree that it might not be possible to address this 100% of the time once themes and user styles come into the picture. At a quick glance though it appears those margins are coming from the My first thought was to add another Given the resets appear to be for specific elements e.g. Through efforts elsewhere to add custom colors block support etc, the need to be able to target specific selectors through the elements API was discussed briefly. It's still a potential option outlined in #33255. With an extended elements API, we'd be able to target the above list of elements only when they were the first or last child in a column. Obviously, this isn't a solution for right now but it might be something to consider moving forward. |
It's tricky to figure out the right user experience here 🤔 I've added a separate background color to the individual Column blocks to make it even clearer: I have a black background on the Columns, white on each inner Column, and pink on the paragraphs. In all cases, padding is set to
In both cases you can see that there is, correctly, 0 padding on the Columns container itself. But there's extra space within the individual Column blocks, caused by margin applied to all of the paragraphs. This could be confusing to a user who expects this setting to eliminate all top and bottom spacing before/after their column content. It's not 100% clear to me what to enforce here. I understand this could be confusing to a user, especially since you're not able to manually control the margin on Paragraph blocks. On the other hand I'm tempted to say that extra spacing is Column spacing, not Columns spacing. You'd get a similarly confusing experience if you were trying to set I guess a bigger question for what 'padding' on the container means -- should it control padding around the container itself, or is it more useful for controlling the padding of all of the inner Column blocks at once? I was thinking of it as the former, but I could see why the latter would be useful. What do you think?
Thanks for this! This would be cool, and we'd definitely want to do this when setting padding on an individual Column, if not also for Columns. For the container I could see it argued either way. Regardless, I'm inclined to wait until we can use something like this to override it consistently for styles set through block supports/theme.json etc. Does this seem like a blocker? |
|
Great discussion, everyone!
Personally, I think it should control the padding around the container itself, for consistency with block spacing. The main thing I like about padding paired with block spacing is that we can now have consistent spacing altogether with a background colour, e.g:
I just had a play with the Group block, and it has the same kind of issue with a paragraph block inside it when you go to reduce the padding down to very small numbers (though
I think because of the consistency with the Group block's behaviour here, I'd lean toward us landing this PR and looking at how to deal with Paragraph margins (and the context within which they're set) separately. |
I agree -- consistency seems the best guide. I also think the background color/external padding controls working together the way you describe is very useful, and it's of course still possible to control individual Column padding.
Thanks for further testing with the Group block @andrewserong! +1 to what you've said here. It sounds like there isn't objection to moving forward? I'll let this one sit to give others a chance to weigh in. |
Thanks for following up Staci, I certainly don't object! 😄 I think it'll be good to get this one in, but yes, good idea giving folks a chance to weigh in if there are any objections. |
| @@ -10,7 +10,7 @@ | |||
| flex-wrap: nowrap; | |||
| } | |||
|
|
|||
| &.has-background { | |||
| &:where(.has-background) { | |||
There was a problem hiding this comment.
Just checking something. When I set padding in my theme.json via styles
"styles": {
"spacing": {
"padding": "55px",
"blockGap": "66px"
},
...These styles override the body styles of padding: 55px. Block spacing works as expected.
There was a problem hiding this comment.
Good callout. This is correct as I understand it -- these styles will override top-level (body) styles from theme.json, but not theme.json styles for the Columns block specifically, or of course values from global styles/the block controls. I confirmed this is how the Group block behaves as well.
andrewserong
changed the title
Part of #24874
Description
Opts into the padding block support for the Columns block (note that padding is already available for individual Column blocks).
I chose not to add padding to the default controls in the Dimensions panel, since it does not strike me as an especially commonly used tool, but it can be easily added.
There may be an argument that it could be added here as axial padding only; I've left padding for all individual sides, as it seems reasonably common to me that a user might want to control top & bottom padding separately.
How has this been tested?
Manually.
theme.jsonScreenshots
Here the Padding control has been added from the + menu
Padding seen on the frontend.
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).