Columns: Update columns to allow full height stretch when vertically aligned #32909
Conversation
aaronrobertshaw
added
[Block] Columns
|
Size Change: +378 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
aaronrobertshaw
changed the title
|
Thanks for working on this one @aaronrobertshaw, I like this change, and it feels like a better approach for the alignment (we want to align the child items of the column rather than the column itself). Unfortunately it looks like this introduces an issue for horizontal alignment with blocks like the image block. Before this change, with the image left aligned, the inserter or block to the right of the image will move around the image: column-flex-item-before.mp4After this change, the inserter doesn't move relative to the size of the left aligned image, and when I insert a paragraph block, it sits behind the left aligned image: column-flex-item-after.mp4I'm guessing that the float-based alignment doesn't play nicely with being a flex item? Other than that, the change works nicely for retaining the height of the column and the column background while aligning the items within it. I can't quite think of a simple workaround without adding an additional container? |
|
Appreciate the detailed review as always @andrewserong 👍
I actually see different behaviour when I run trunk. The alignment of an image block as an immediate child of the column doesn't work there either. This is what I experience with the current layout/styles on
That's my guess as well as to the root cause. The issue on trunk might have similar origins just further up the tree. I've pushed a comment that forces an auto margin left on an immediate child of the column with the After applying the new style to force right alignment I get the following behaviour:
I suspect we might also have some theme specific issues with this, so it could use further testing on that front. Which theme were you using in your videos? I couldn't replicate the issue with the image's resizable container overlapping the quick inserter. |
|
I've found that I can replicate the issue with the inserter being overlapped when using the Maywood theme. I'll test further with that. |
aaronrobertshaw
force-pushed
the
update/columns-vertical-alignment
branch
from
efac62f to
dc83d3d
Compare
|
It appears that the overlapping issue comes from the Looking into this has highlighted the change to flex display also prevents the inserter DOM element getting a width resulting in the inserter icon display off to the left of where it should be. This effects all themes for me. I've tried switching a few different approaches using flex or grid without a clear winner. A few additional tweaks have been pushed, along with rebasing this one. Let me know if you have any ideas as to an alternate approach, I'm running out of ideas. |
|
Thanks for digging into this one @aaronrobertshaw (and figuring out that I was using the Maywood theme!). It is a tricky one.
As a workaround, that actually sounds quite reasonable to me. Since this PR could introduce a regression for existing layouts that folks might be using with wrapped content, I'm wondering if it'd be worth moving the On my test site it's working nicely using an additional group block for the situation where I might need the text to wrap around the aligned image element: (The unusually large vertical margin is from the Maywood theme's styling):
I think this feature is a matter of striking a balance between trade-offs, unfortunately! Being able to have the column background color reach the full height of the columns block unlocks a number of good options for pattern designs (e.g. vertically centered content in a row of columns with striking background colors) that we mightn't be able to achieve otherwise. And with this change, it's still possible to do the floated alignment with wrapped paragraphs if the user adds an additional container block. So, I think if we can see about only setting the column to flex when a vertical alignment is applied, hopefully that'll be a decent enough trade-off. We'll then want to test this across a number of themes / block combinations to make sure we're happy with the trade-offs? One other tiny note, it looks like this needs a rebase now after #31816 landed (apologies!) |
aaronrobertshaw
force-pushed
the
update/columns-vertical-alignment
branch
from
dc83d3d to
dc82868
Compare
This is actually a thought I had while revisiting this as well. I missed adding the style back in after continuing to experiment, looking for alternative solutions. Thanks for catching this!
Agreed. I believe this is good option for the moment as it does, as you say, unlock a lot of design possibilities while still allowing the wrap around effect albeit with the use of the group block.
I rebased this prior to adding back in the style to only apply flex layout when column is aligned vertically. |
There was a problem hiding this comment.
This is testing nicely for me @aaronrobertshaw, and in a multi-column layout where a user might want an active vertical alignment setting on one or more columns, while keeping one of the columns without that alignment setting, the floated left/right alignment is working correctly. For example, this kind of layout works (though the Columns blocks' mobile breakpoints don't play that well with it):
I think the current implementation in this PR strikes a good balance of trade-offs, enabling the desired styling, while minimising impact for left/right alignment, so this LGTM, and I tested with half a dozen themes across Safari, FF, Chrome, and Edge. But, it might be worth getting another pair of eyes from a designer, too 🙂
|
Appreciate the thorough testing across themes and browsers @andrewserong. 🙇 It is certainly appreciated given the potential impact of such changes. To that end, I agree, it would be handy to get some extra eyes on this. @jasmussen or @paaljoachim if you have some spare time, would you be able to take a look at this and let me know if there is anything that I've missed. Thanks in advance 👍 |
|
Hi, there. Have you considered adding a Full height aligment option to the toolbar Something that we did for the cover block . |
|
Thanks for the suggestion @retrofox 👍 I did consider a similar approach early on. It felt like it was adding extra complexity to the controls and toolbar when the user is already more than likely requesting full height behaviour. The issues @andrewserong highlighted in regards to wrapping inner aligned/floated elements would remain either way. My understanding of our general approach to these sorts of issues is to err on the side of providing sensible defaults and minimising the need for additional controls. It feels as though full height columns when a background is set is a fairly sensible or expected behaviour that also avoids the need for an extra control. I'm more than happy to get further opinions on this and update as needed or include the extra control/classes etc. |
|
Coming back to this after some AFK, thanks for the ping! Thanks for working on this, it's a fix we want. My immediate and only question is this part of the code: It sounds to me like floats are the primary challenge that makes this be a conditional, correct? Specifically thinking of this comment:
Floats have been challenging in flex containers in the past, and a recent Mostly I'm concerned about code complexity and a proliferation of edgecases by changing the display mid-stream like this. Do these concerns make sense or am I missing a beat? Thanks @andrewserong and @aaronrobertshaw |
|
Here's an adjacent conversation about floats: #33332 (comment):
|
Yes, I believe so.
Those concerns definitely make sense to me! I know what you mean, each time we add edge cases, it makes the CSS harder to read and means we've got additional cases to test whenever we make changes. So good to avoid if possible. It's a tricky one with the Column block, because I do like the idea of ditching floats altogether and just letting flex work the way it needs to, but we also want to avoid causing regressions for columns out in the wild where folks might have used left or right aligned images as direct children of the column. So, I think the trade-off here is how much backwards compatibility we want to maintain (and whether we're happy with the potential impact of switching the column to flex). @jasmussen would your preference be for us to keep flex and ditch the rules to support floats, or do you have another approach in mind? Damian's suggestion of a full height alignment option is also a compelling idea, but could also wind up adding extra complexity, too? 🤔 |
|
It's a doozy, I see the problem. Each column is The curious thing here is, in my testing (block themes only), floats don't work at all inside columns even now. Which we either embrace and disable the float button, or fix with column specific float overrides such as this PR adds. But if we do that, add float overrides, couldn't we just embrace flex for column's (singular column blocks) entirely, and remove the conditional? |
Interesting! Re-testing on trunk now, I can't reproduce floats working with paragraph text wrapping around floated image blocks within a column block, either. So if that's the current behaviour, then yes, maybe it'd be better to embrace flex entirely. My only reservation is making sure that we don't accidentally introduce regressions for columns that are out in the wild. But I agree, it'd be nice to embrace flex entirely if we can. We've been focused on some other PRs this week, but I'm happy to do a bit more testing on this next week to see what's feasible! |
I believe your earlier testing was with the Maywood theme @andrewserong. On trunk, quickly flicking the theme over to Maywood, you can see an image floats still within a column allowing the following paragraph block to wrap. Other themes out of my limited library that allowed the float wrapping were; Ambitious, Hever, Seedlet, TwentyTwentyOne (non blocks), TwentyTwenty, TwentyNineteen and so on.
I like the consistency of embracing flex and dropping support for floated elements as immediate children of the column. There should still be the work around of enclosing whatever you want to wrap within a group block. That still leaves us with the problem of backwards compatibility for all the themes out there that use floats for left/right alignment. The current CSS rule limiting the display of flex to only vertically aligned columns does reduce the potential breakage for columns and themes already out in the wild. As much as I'd like to drop it, I don't think we can. |
|
Thanks for the reminder, Aaron, yes I was testing with Maywood.
Agreed, it does feel safer to preserve that. |
The previous testing of various themes would indicate to me, that comments elsewhere stating floats are not allowed in columns isn't exactly accurate or at least is ambiguous. Whether columns should be exclusively flex containers and not support float alignments of inner blocks, is perhaps a separate question. I think of this PR as correcting or bringing the behaviour more into line with existing use and expectations, rather than drawing a line in the sand over how columns should be laid out. Can this land as is, given it does improve the columns block? Would it prevent a more detailed discussion of converting columns to flex only in a separate issue/PR? |
Since it would be easy to follow up on, nothing beats trying it out in practice. 👍 👍 |
aaronrobertshaw
force-pushed
the
update/columns-vertical-alignment
branch
from
dc82868 to
14acfda
Compare
|
@aaronrobertshaw Looks like this one could merge, are you waiting on any more feedback? |
|
Thanks for checking in @apeatling. I was waiting to see if the work around flex via the layout support might have been adopted for the Columns block. At this point, if there are no objections to the current approach, I'll rebase it, give it another test then can merge it. |
aaronrobertshaw
force-pushed
the
update/columns-vertical-alignment
branch
from
14acfda to
67924d9
Compare
aaronrobertshaw
force-pushed
the
update/columns-vertical-alignment
branch
from
67924d9 to
63d1dc3
Compare
|
After some further testing on this I found;
To address the new issue I've added a Before (Trunk)Screen.Recording.2021-09-22.at.10.58.57.am.mp4After (This PR)Screen.Recording.2021-09-22.at.11.00.41.am.mp4 |
|
Thanks for following up on this one @aaronrobertshaw! I just took it for a spin, and noticed that with this PR applied, adjusting the vertical alignment also appears to affect the spacing between blocks. Here's a before / after:
I'm testing with |
|
Appreciate the quick turnaround in testing @andrewserong 👍
I don't believe this one will be theme-specific. We are having to rely on setting a flex layout for the column. As far as I'm aware, flex layouts don't support margin collapsing and it is the lack of margin collapsing that results in the increased spacing between blocks. In other words, all the flex items' margins are actually honored. I'm not having much luck working around this. So open to fresh ideas if you have any? |
Thanks for the explanation surrounding margin collapse, that makes sense why it's behaving differently! I just did a bit of hacking around, but unfortunately I haven't been able to come up with a workaround, either. It'd be a pity to mess with the vertical rhythm of an existing theme, but I suppose at least the behaviour is guarded behind having a vertical alignment selected? I think the thing to explore next is whether or not the trade-off is appealing to designers or folks with a bit more experience working with theme development. Some not particularly useful dead-end ideas:
Overall, I like the idea of using flex for the column (and it feels like the right way to go about it), but it'll be interesting to see how much themes are relying on the margin collapse and whether or not it's a deal breaker 🤔 |
|
Probably highly dodgy, but we could play with Here's what I did to get the above effect: P.S. I used |
An interesting idea. My concern would be how that would play with blocks that the user has set manual margins on e.g. via blocks supports, or maybe a theme author by theme.json where the value doesn't match the global. |
|
I was testing this again last night and couldn't find a way around the lack of margin-collapsing for flexbox. But it made me think: what if we gave column blocks a |
I haven't found the solution yet either. Once we have increased adoption of block gap or margin support, the extra spacing due to the lack of margin-collapse for flexbox could be configured away. I think this is an acceptable price to pay for users wanting to be able to have the full height background in columns. Even as it is now, if that extra spacing is a problem, you could wrap contents in a group block and regain that margin-collapse. Obviously, it's difficult to expect users to know that's an option. It is for an edge case and the desire to have full height backgrounds appears to be of more concern.
Doing this would potentially introduce a regression on blocks already out in the wild. In particular, those columns that have floated/aligned inner content. #32909 (comment) |
No workaround for this design need with native blocks
PR fix attempt already 3 midsummers old now…
|
Fixes: #32907
Description
Notes
The ability to set a background color on individual columns was recently added. Prior to then columns simply set
align-selfstyles to achieve the vertical alignment.This PR changes columns to be
display: flex;and vertically align their children within them in line with the column's vertical alignment selection. This is working with multiple inner blocks or nested columns.How has this been tested?
Manually.
Screenshots
Types of changes
Bug fix / enhancement.
Checklist:
*.native.jsfiles for terms that need renaming or removal).