Use grid-auto-rows to ensure min height of new grid rows.
#61383
Conversation
tellthemachines
added
[Type] Enhancement
tellthemachines
requested review from
noisysocks,
jasmussen,
ramonjd,
andrewserong and
fabiankaegy
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +205 B (+0.01%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Oh, interesting problem! In testing, the included rules feel like they could be a bit too opinionated potentially, as I found that it's always extending the height of the grid even when that's not expected.
For example, I added a Grid block with some child images, where the first row of images are fairly short and the second row are tall. On trunk, the first row is the height of the content. With this PR applied, there is unexpected whitespace in the first row, that ensures all rows are the same height:
| Trunk | This PR |
|---|---|
 |
 |
In the above example, there is no set number of rows, and the grid is set to Manual and 3 columns.
A couple of questions:
- In which contexts do we want the rows to extend their height?
- When will we want rows to be consistent heights, and when will we want to allow variable heights?
These are good questions! I'd love to hear @WordPress/gutenberg-design opinions on it. I wasn't really thinking about giving all the items the same height, rather making sure there's always a minimum height to the auto rows so they don't look weirdly flattened. I might adjust the |
I suppose another option could potentially to be (obligatory note that I am not a designer 😅), what if the minimum height of the row is an editor-only style while the block is selected, to make it easier while folks are building out a grid, but otherwise doesn't affect the real layout output? |
Hmmm yeah my main concern is if folks are trying to make a block bigger by spreading it across more rows and then it doesn't work in the front end, it'll seem like something's broken. I'd expect users to adjust the size of their blocks to whatever they see fit in the editor, and front end to reflect that. I don't think it's possible to mess up terribly with this because new rows will only be created if there are blocks to occupy them (unless user sets rows explicitly in the manual grid, in which case I assume they know what they're doing) |
Good point. I suppose whenever picking defaults for the site output, it's hard to find a reliable default because it means making some assumptions about the nature of the content that someone's building, which is very hard to predict. I.e. someone could conceivably be building a pattern or layout for high density tabular-like data with the Grid block, where there's no spacing between rows, and the height of each row is the height of the font size. So it could be hard to come up with a default min height that doesn't potentially conflict with various use cases. But I very much see your point that when someone's dragging the size of row span, they'll be thinking of the row items as being "solid" and it would be a surprise if that's not how they really exist on the site frontend, as you mention. Perhaps not for now, but I wonder if longer-term, it'd be helpful for the minimum height of all grid items (the value within |
|
Just realised I hadn't pushed the update changing the style to |
|
Flaky tests detected in 60f5e5b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8979340678
|
There was a problem hiding this comment.
Unfortunately, the minimum still doesn't feel quite right to me. When creating a grid with a row that has paragraph content of a single line height, the 50px height is unexpectedly tall:
| Trunk | This PR |
|---|---|
 |
 |
One idea, would it be possible to use a minimum height of 1lh instead? (1 line height). That seems like it could resolve the issue for single row paragraphs:
Also, it means if folks increase the line height on the group, it'd naturally increase the minimum of the rows, too. I.e. here's the same with a line height of 3.5:
Alternately, would it be worth looking into outputting this rule only when needed? I.e. when there's a block with a row span that extends beyond the number of rows present in the layout, or does that get too convoluted?
|
Tricky! Having a 0-height row is a totally valid choice, no? If so then the problem isn't necessarily that the row has no height, it's that the editor's UI doesn't allow you to ergonomically work with 0-height rows. In #59490 I made it so that the cell that |
|
Thanks for the feedback, folks! I think we could try with a smaller minimum. This is one of those changes it would be good to have a variety of folks testing to see what might work best.
Hmm this could get complicated because this rule is output on the grid block, which doesn't know about its children's styles, but also because it would mean knowing not only the attributes but the position of the child block in the grid. It would be preferable to output a blanked rule that works in all cases 😅
Theoretically yes, but I can't think of any scenario where someone would want a 0-height row 😅
That's a possible alternative, I'm not sure how good it is to have editor not matching front end though. Again, wider testing and feedback would be helpful here. |
|
Ok I've updated this to use |
|
Actually this would be a good one to get feedback from @WordPress/outreach! |
|
While testing the grid, I noticed the following issue. Sorry, I don't know if I should show this in the Slack channel #outreach. |
|
It works as the PR intended:
I'd tend to think that this is useful, but I'd also echo Andrew that this could end up being too opinionated. This is one of the reasons why grid is so hard to get right: what's the expected behavior in this case? I'd appreciate broader opinions on this. Good idea to ask in the outreach! |
|
@hagege thanks for testing! That's expected, because when you expand the top left block to span all the columns, it pushes the block that was to the right under it, and that block spans 3 rows, so the grid has to create an additional row to accommodate it. |
|
@tellthemachines thanks for clarification. |
|
I see that there is a reason to ensure that a row is editable in the Block Editor, but collapsing rows are standard behaviour in CSS Grid. Forcing all rows to a minimum height goes against this and would mean that themes built for client projects would usually have to reset this back to standard behaviour. This seems to be counter-productive, both for users and implementers, if it affects the frontend layout and goes against common goals. There will always be an argument against any arbitrary |
|
After reading this thread, I agree with the commenters that an arbitrary min-height will cause more problems (and hacky CSS overrides) than it solves. I still think this is the best approach:
Show the hidden row placeholders when a block is selected and when the user clicks out, they collapse. If feels like the leanest/truest version of what is happening. |
|
Thanks for the feedback everyone! There's some good reasoning to leave things as they are and not change grid default behaviours. I'll go ahead and close this for now; we can always revisit later. |
What?
Fixes an issue that is becoming more visible with the work on #60986: when a grid item spans several rows, but there aren't otherwise enough items in the grid to occupy that number of rows, the auto rows created to support the multiple-row item are collapsed to 0 height. This looks quite odd, and is unlikely to be what's expected when resizing an item to take up more vertical space:
This PR adds a default value to
grid-auto-rows, so that auto-created rows always have the same minimum height as the other grid items:Testing Instructions
To test, add the following markup to a post:
test markup
Then set the alignment of the grid to full, and resize the window so that all grid items fit on the same column.
Testing Instructions for Keyboard
Screenshots or screencast