Columns: Avoid using CSS variables for block gap styles #37436
Conversation
|
Size Change: -601 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
Not sure if I captured your idea correctly here @youknowriad I've made a start on removing the CSS var for the Columns block and things appear to work okay so far. I won't be around for the next two or so weeks, so feel free to destroy this PR if it's not relevant. |
|
This captures my idea yes, I kind of expected we'd be able to remove more styles form the block buy maybe there's some special cases for this block that we need to account for (responsiveness...) I guess we should test this properly and see if everything still works as expected for the columns block (responsiveness, column widths, column count...) |
|
I'm partially AFK today, so I can only give some high level thoughts, hopefully they are useful.
It is probably a good idea to move forward with this one if it's necessary for #37360 to land. But it really could use good testing across existing patterns or designs that use columns. |
youknowriad
force-pushed
the
try/no-css-variable-block-gap
branch
from
8ce5950 to
87c99bf
Compare
andrewserong
force-pushed
the
try/no-css-variable-block-gap-columns-block
branch
from
32e3925 to
7ec7e96
Compare
|
Thanks for taking a look @jasmussen! While Ramon isn't around, I thought I'd see if I can help progress this PR 🤞
Yes, for the moment we still need it to account for the use case of columns that don't wrap, so I think at this stage our objective is to try to keep close to parity of existing functionality.
Totally. I also like the points made in #37252 and in @aristath's exploration into minimum column width in #33330. For our purposes here, there was an issue with trying to keep the existing "tablet" breakpoint CSS as-is — it turns out that the In the "tablet" breakpoint, set
This will need more testing, of course.
Very much this! I'm curious to see how this fares with real world content. If anyone can think of a better approach for the "tablet" breakpoint at this stage, let me know! I've also added in a default |
andrewserong
added
[Block] Columns
|
I've just switched this PR out of draft. I think it's ready for review and more feedback, but it could use a lot of testing with different patterns / themes, etc to see whether or not the change is disruptive, or if it needs further refining. |
|
Thanks for all the thought. Just to be sure I'm reading you clearly, this PR is meant to remove the tablet view for now, right? Because that's what I'm seeing in practice as well. This PR skips it:
Trunk still has the tablet step:
To be clear, I personally think that's acceptable if we can test sufficiently any existing patterns that might rely on the tablet breakpoint. Also because this effectively fixes #37252, CC: @scruffian and @MaggieCabrera. For the longer term discussion, it would be really nice if we could find some way to have the columns be equal width, as the arbitrary widths aren't great. Anything we can do here, even if it needs moving towards grid, might be worth it. Was this helpful? |
It sure was, thanks @jasmussen!
Yes, in practice we're effectively removing the tablet view, but still allowing wrap at that breakpoint if the content requires it, to hopefully smooth over that viewport width. 🤞 We'll find out in testing with real content whether or not the CSS at that viewport meets out needs. @scruffian and @MaggieCabrera if either of you get time to test this PR with the patterns/themes you're working on, we're very happy for feedback on how to best handle things at that viewport width. After tomorrow, I'll be AFK, too. Feel free to pick this up if anyone wants to try to get this PR in sooner, otherwise Ramon or I will be able to look at it again early in the near year. |
|
Thanks for the clarification! The wrapping on the tablet breakpoint could be a challenge, I imagine that's what's causing the arbitrary width columns in your GIF. As this gets more testing, we might just want to turn that off as well, remove the entire tablet behavior cleanly, then revisit when we can. |
|
I added a commit which removes the difference between the tablet and mobile behaviour - now the columns stack on tablet too. I also tested with two themes that use columns. Here are the differences: Stewart: After: Remote: After: IMO this is an improvement, but I am concerned about people who expect the current layout on tablet which will now break... |
I think both behaviors are desirable in different contexts. I agree with @jasmussen here:
I think it's best that we remove the tablet styles that so far themes would have to override and let themes add it if they need it while we look for a better way of handling this kind of situation. |
|
The price of removing the break is we need to thoroughly test any patterns, including older ones, to see if results are mostly acceptable. |
|
Thanks everyone (and @scruffian for removing the tablet break point!), I agree, I think that's a better path forward. I'm wrapping up for the year shortly, so I'll leave this PR for everyone else to test (and feel free to merge if it winds up looking viable, of course). Happy to help test / follow up again in the new year. |
youknowriad
force-pushed
the
try/no-css-variable-block-gap
branch
from
87c99bf to
58c63eb
Compare
ramonjd
requested review from
ellatrix,
oandregal and
spacedmonkey
as code owners
|
Thanks for testing so thoroughly @stacimc
This looks like a theme-specific phenomenon. It seems as if TT1 is applying the top and left offset for a tablet-like breakpoint (min-width 652px) .is-style-twentytwentyone-columns-overlap .wp-block-column:nth-child(2n) {
margin-left: calc(-2 * var(--wp--custom--spacing--horizontal));
margin-top: calc(2.5 * var(--wp--custom--spacing--horizontal));
z-index: 2;
}
Looks like this is also very theme-specific. TwentyTwenty places a few negative top margins on Columns Blocks at various breakpoints, and when a wide or full alignment is active. This one (min-width 700px) appears to be related to the effects we're seeing .wp-block-columns.alignwide + .wp-block-columns.alignwide,
.wp-block-columns.alignfull + .wp-block-columns.alignfull {
margin-top: -6rem;
}Given the specificity to the theme, and the unique effect that they're aiming for I wouldn't catalogue them as blockers for this PR. I don't see these effects on these patterns at the same breakpoint when changing theme for example.
I'd rely on @scruffian's advice, but seeing as they're very specific to the themes, perhaps we could address them in the themes themselves? |
|
We've made some final changes and have been testing this PR today. Aside from some theme-specific issues, things are looking pretty good in my opinion, whatever that's worth. I've followed @stacimc's test plans. There are probably, as usual, some unknown unknowns, but I haven't managed to come across any blaring sirens in my testing. If you get time, it would be great to get your opinion on whether it's okay to go ahead and, more importantly, backport this one. |
There was a problem hiding this comment.
Nice work 👍
This is testing pretty well for me. Overall my impression is that the spacing of columns is improved in a few themes without any major breakage. I did encounter the theme-specific issues others have already noted.
- Try adjusting the block gap at the parent Columns block level
I have one small issue/question. Should this be opting into the blockGap support for the Columns block? The GIF in the PR description along with the quoted test instruction suggest it should be.
If we are, would we be removing the margin block support or keeping both despite the overlap?
✅ Tried columns blocks with varying numbers of columns, widths and stacked on/off
✅ Tested all the per-column vertical alignments
✅ Confirmed responsive behaviours for tablet and mobile breakpoints
✅ Tried adjusting block gap at columns block level (after tweaking block.json to opt-in)
✅ Tested with various themes and patterns
Test themes:
- TwentyTwentyTwo
- TwentyTwentyOne
- TT1 Blocks
- TwentyTwenty
- TwentyTwenty Blocks
- TwentyNineteen
- Hever
Test patterns:
- Three columns of text
- Three columns with images and text
- Pricing table
- Two columns of text and title
- Two columns of text with offset heading
- Overlapping images
- Overlapping images and text
|
Thanks for the thorough testing @aaronrobertshaw
Looks like blockGap support was deactivated as part of removing the block gap style CSS variable in Avoid using CSS variables for block gap styles #37360 in this commit. I didn't get around to updating the test description today. Will get onto that now. Sorry for the confusion!
That's a great question. In my limited view, I don't see why we couldn't keep the axial top/bottom margin. It seems to be uniform in it's application.
Others might have a different opinion. Let's see if @youknowriad thinks this PR is okay to approve and backport this PR today 🎉 |
My concern was for mobile/tablet and once the columns start stacking. Would we not end up with both the block gap and the margin being applied? As a result of Flex not having collapsing margins. |
|
This PR looks good to me. Should we bring back the blockGap support as well (it's removed on trunk) |
I've been testing this. It looks pretty stable and work as expected (that is, no real unexpected changes since I last tested in December 2021)
I'll re-add it.
Thanks for reminding me about the lack of margin-collapsing for flexbox. In this PR there is overlap of margins between columns. I've had a play around, and to me it seems okay in the editor as I can overcompensate to get the effect I want using a combination of axial margins and gap. This PR locks the columns layout into flex for now via Is there something else we should be testing? I recall you mentioned the possibility of regressions in blocks already out there? |
The column stacking part of the equation isn't an issue. I confused the
From memory, that comment was related to the content inside a column. Column blocks out in the wild potentially have floated contents so forcing them to display as flex containers could break their layout. This PR is only directly related to the parent Columns block so shouldn't be an issue. Sounds like this is ready to go. 🚢 |
|
I was thinking about this and 5.9. 5.8 doesn't have gap support at all in columns meaning if we don't land this in 5.9, it won't be considered a regression like in the Gutenberg plugin where the block used to have block gap support. So given potential impact of this PR, I was thinking that maybe it's fine to just land it in Gutenberg plugin for now? Thoughts? |
Thanks for your guidance on this one. Landing in the plugin sounds like a safer bet. I know there's some pressure in relation to 5.9 and it would help to have the breathing space to monitor/debug/garner feedback just in case we're breaking unknowns. |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
|
Hi all. My site is ugly now due to this breaking-change PR. There were 2em of gap between columns but now it's 0.5em. 5.8 doesn't have a block gap support yet so can't change in the editor. Is there a workaround for 5.8? I am rolling back to 12.3.2 but it's not a long-term solution. Thanks! |
Hi @umhan35 Sorry to hear that! Thanks for commenting here. The CSS var was creating a bunch of conflicts, creating more problems than it solved. This PR was a result of limiting it to layout-based blocks. Are you able to edit your site's theme.json (assuming you have a block-based theme like TT1 Blocks )? You could try enabling blockGap support for the columns block. So, ...
"settings": {
...
"spacing": {
"blockGap":true,
...
},
...
"blocks": {
...
"core/columns": {
"spacing": {
"blockGap": true
}
}
}
}and then head to the editor and adjust the block gap setting for that particular block:
If none of these options are available to you then I think you've done the right thing in rolling back for now. WordPress 5.9 will be released early this year so 🤞 this will fix this particular issue for you. Hope that helps for now. Cheers! |
…k from #37436 which forces 2 columns rather than a total stack
…k from #37436 which forces 2 columns rather than a total stack
…k from #37436 which forces 2 columns rather than a total stack
Description
An experimental PR to make the Columns block gap behave should we remove the CSS var from the block styles. See:
In short, we're forcing a flex layout via
__experimentalLayoutand related properties. The rest is allowing flexgapto do its thing.This branch
Trunk
Changes included
50%width at that breakpoint (the 50% rule had to be removed as we can't calculate the appropriate gap without a CSS variable. See: Columns: Avoid using CSS variables for block gap styles #37436 (comment))!importantCSS rules to smooth over differences between the Layout support's styles and the current styles for the Columns blockHow has this been tested?
25%,50%,25%) and hard-coded widths like200px.Types of changes
Bug fix with a potential breaking change for tablet viewport widths.
Checklist:
*.native.jsfiles for terms that need renaming or removal).