Try adding a grid layout type #49018
Conversation
|
Size Change: +1.72 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This looks like a great feature to begin with @tellthemachines! Love the idea of using minimum column width with a max of 1fr, the auto-fill behaviour tests very nicely for me across a range of viewport sizes.
Also, by conditionally outputting grid-template-columns only if minimumColumnWidth is set, it means that we're not boxing ourselves in for future rules that might require a different value for grid-template-columns 👍
Just left a comment about the base styling rules, but this is looking really promising to me as a useful MVP. It's also really nice seeing the Grid option in the variations list, it's going to be a fun tool for folks to play with 😀
lib/theme.json
Outdated
| "slug": "grid", | ||
| "className": "is-layout-grid", | ||
| "displayMode": "grid", | ||
| "baseStyles": [], |
There was a problem hiding this comment.
Looks like we might need to include the margin: 0 rule from baseStyles used by the flex layout, too. Otherwise the user agent stylesheet margin rules for e.g. p tags apply, affecting the gap:
| @@ -1341,8 +1341,7 @@ protected function get_layout_styles( $block_metadata ) { | |||
| $base_style_rules = _wp_array_get( $layout_definition, array( 'baseStyles' ), array() ); | |||
|
|
|||
| if ( | |||
| ! empty( $class_name ) && | |||
| ! empty( $base_style_rules ) | |||
There was a problem hiding this comment.
It looks like this change is to allow the block of code to be reachable if $base_style_rules is empty? Tiny nit: would it be worth retaining a check, but instead have it check is_array( $base_style_rules ) since the code below will attempt to iterate over $base_style_rules? (This is just to make sure that anything setting null or false won't throw an error)
|
|
||
| return ( | ||
| <UnitControl | ||
| size={ '__unstable-large' } |
There was a problem hiding this comment.
Should we add a label to this component? E.g. label={ __( 'Minimum column width ' ) }?
There was a problem hiding this comment.
Yes, that's a great idea!
|
Thanks for the feedback @andrewserong ! just pushed an update with an alternative, static column number style. It behaves exactly like Saxon's demo in that the columns just squish on smaller viewports, with no reduction in column number. This is probably something we'll want to iterate on 😅 and it might be nice to give container queries a try in this context. My main question is would it be best to provide responsive behaviour via queries behind the scenes or configurable via UI? cc @SaxonF @WordPress/gutenberg-design |
|
Flaky tests detected in 099db1e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4497336703
|
| > | ||
| <Path d="m20.30137,-0.00025l-18.9728,0c-0.86524,0.07234 -1.41711,0.79149 -1.41711,1.89149l0,12.64468c0,0.6 0.73401,0.96383 1.0304,0.96383l19.67469,0.03617c0.29639,0 1.0304,-0.4 1.0304,-1l-0.03576,-12.7532c0,-1.1 -0.76644,-1.78297 -1.30983,-1.78297zm0.52975,16.60851l-19.99654,-0.03617c-0.29639,0 -0.92312,0.36383 -0.92312,0.96383l-0.03576,12.68085c0,1.1 0.8022,1.81915 1.34559,1.81915l19.00857,0c0.54339,0 1.45287,-0.71915 1.45287,-1.81915l0,-12.53617c0,-0.6 -0.5552,-1.07234 -0.8516,-1.07234z" /> | ||
| <Path d="m42.73056,-0.03617l-18.59217,0c-0.84788,0.07234 -1.38868,0.79149 -1.38868,1.89149l0,12.64468c0,0.6 0.71928,0.96383 1.00973,0.96383l19.27997,0.03617c0.29045,0 1.00973,-0.4 1.00973,-1l-0.03504,-12.7532c0,-1.1 -0.75106,-1.78297 -1.28355,-1.78297zm0.51912,16.60851l-19.59537,-0.03617c-0.29045,0 -0.9046,0.36383 -0.9046,0.96383l-0.03504,12.68085c0,1.1 0.78611,1.81915 1.31859,1.81915l18.62721,0c0.53249,0 1.42372,-0.71915 1.42372,-1.81915l0,-12.53617c0,-0.6 -0.54407,-1.07234 -0.83451,-1.07234z" /> | ||
| </SVG> |
There was a problem hiding this comment.
I improvised a grid icon here but we'll probably want a proper one at some point 😄
|
I put up a draft that applies the static grid layout to Post Template in #49050. I think that highlights the potential problems of having a purely static column number; it definitely feels like there should be some responsive behaviour in place. |
|
Exciting stuff @tellthemachines ! This looks like it could be a really great starting point for grid and lead into on canvas editing. I kind of see this new layout type as a "snap to grid" option that replaces the need for the old columns block. +1 to gap/block spacing driven by spacing scales.
+1 thinking we add columns range to dimensions panel, either replacing width or in addition to.
This is tricky as we want to lean into intrinsic layouts where possible. If we wanted to adopt similarish behaviour to the columns block we could either:
|
Agreed. One of the things I really like about the Or another (potentially improbable) idea 😅, what if the column number could be treated as "fuzzy" or "explicit" (or something like that), and if it's fuzzy, then it's actually just setting a |
|
Exciting stuff! A quick GIF showing the inspector controls toggling responsive on/off, and tinkering with the min-width control for child blocks:
Already starting to look like a better alternative to the Columns block. Small detail, I couldn't click/drag inside the input slider to increment/decrement. We might also want a input+slider combo for this. Also, thanks for using the 40px controls from the start 👌 The term "Minimum width" is good. To Saxon's point, it leans into the intrinsic responsiveness stuff, and it allows the block to do its magic and stay responsive. There's a visualization challenge to think about in the future, which is shared with the Row block's width controls: a min. width doesn't have a discernible effect in the canvas until the value exceeds a threshold of needing to wrap. It's visible in the GIF: I change the numbers but there's no visible effect, for a while, in the canvas. This is also a challenge if we ever want to add resize handles in the canvas — not sure we want that — but something to think about. A solution might be a visualizer, similar to the margin visualizers we have: a blue overlay shown while interacting with the control. The main question seems to be: what's the first version of grid we land? Is it a block, or something else? If we go for a new block, I would not add a responsive toggle. Fixed-width columns are handled by the Columns block, having another place might add confusion. By surfacing child block width controls like Saxon suggests, it might also be possible to accomplish this in a more buildery and flexible way. (Also worth noting, we will almost certainly implement a generic query editor in the future. #19909 is out of date, but it outlines the value proposition, and suggests a single interface that works across blocks. So with few exceptions, we should avoid per-block responsive implementations that aren't intrinsic.) A benefit of making it a block & group variation, beyond organically growing to support new features without back compat concerns, is that we could potentially surface this variation picker on the Post Content block, allowing your main canvas to become grid based. The challenge here is to boost our confidence that a new block is the right solution. To do so, I'd keep each feature addition as minimalist as possible, avoid the responsive toggle, and see what patterns our theme devs might be able to accomplish with the new min-width control. A next step could be Saxon's suggestion of child block width controls. The combo might be powerful. But perhaps we should put the new variation behind an experimental toggle until we're sure? Mainly we want to avoid adding a grid block and then removing it again because of something unforeseen. Another path forward could be absorbing the "Stack on mobile" aspect of the Columns block, perhaps eventually refactoring the whole thing. Conceptually if columns are the main implementation piece of the grid spec, the "Columns" name might be more obvious for the average user. Back compat and complexity might be a blocker for this, I'll refer to you on feasibility. Thanks for the work! |
|
Great work @tellthemachines! I found what you have so far quite intuitive, especially as a variation of the group block, it felt like a natural edition in the placeholder and sidebar next to the other layout variations. I don't have a great deal to add at this point, it made sense to me. I did find myself agreeing with Joen above:
In the sense that even in the group block variation setup here, it's still duplicative of the columns block and could be confusing?
|
|
Thanks for all the feedback folks!
The width/height panel won't really work for grid children; the only way we can manipulate size on the actual child is by changing the number of columns it spans. Fill/fixed type behaviour has to be defined on the columns template at the parent level. Let's go with just the columns range for now!
I think reducing number as viewport (or container) scales is a better approach, given that a frequent complaint about the current behaviour of Columns is that it goes from x to 1 with no middle ground 😄
That's an interesting idea! It would be tricky to implement. It's probably not an optimum solution for smaller screens though, given if we have any kind of responsive behaviour we couldn't accurately show what a higher number of columns would look like, if that makes sense?
I think the easiest and probably best way to quickly get some real-world use is adding it as a Group block variation.
We could use a feature flag, but that would mean less exposure and less ad-hoc testing. The current implementation - especially if we remove the "non-responsive" variation and go with just the "min-width" behaviour for now - is a pretty minimal baseline that will allow us to build in whichever direction we like, so it's unlikely we'll have to undo it later. I might be missing something so would be great to hear other folks thoughts on this!
It would be worth exploring (again!) how Columns could work with grid but due to the nested nature of the block it will likely be complex to implement, so might not be the best starting point for exposing grid-based layouts to users. An additional thought: I'm pretty excited for Subgrid which is currently being implemented in Chrome. Support for it would be pretty simple to implement on top of this PR: we could add an "inherit" toggle to descendants of grid layouts that would allow them to align with the parent grid. |
@tellthemachines just to clarify my thoughts here. Column span could be seen as a width unit type (like px) even though behind the scenes it's a separate property. Was thinking something like |
|
I've reverted the non-responsive option for now.
It's working for me, though with |
|
Noting that this comment responding to this comment on #49084 mentions it might be good feedback on this PR too. |
|
I've added a RangeControl, but am running into semantics and labelling-related issues 😅 I'd appreciate feedback on. Both UnitControl and RangeControl have the same functionality, so they should have the same label. We could wrap both these controls in a |
|
@tellthemachines Whatever you do, do not use Thanks. |
|
Because they control the same functionality, they can have the same label. Different labels are only required if the fields are actually different. The different semantics (type of input) will be conveyed by the control itself, so there's no need to differentiate. I'll second Alex's comments on aria-hidden, though - using aria-hidden on focusable elements is a poor user experience for screen readers. It's only intended for hiding text nodes. |
Not sure if it helps (or if it's the right approach), but with the HeightControl component, which pairs a UnitControl with a RangeControl, it uses a If we wind up going in a different direction, though, we could always update the HeightControl, too 🤔 |
There was a problem hiding this comment.
Just took this for another spin (I'm so excited about this PR 😀), and had a couple of questions:
- Left a comment about whether or not we should prevent columns from overflowing by nesting a
min()within theminmax()of the rule, to deal with narrower viewports - Quite possibly not a question for this PR, but in a follow-up will we want to add the content justification options here? Though I imagine we'll likely want to try playing around with
justify-itemsfor some fun with how child blocks are aligned within their columns?
As it stands, just the minimum column width alone does feel like it'd make for a useful MVP to me.
| if ( minimumColumnWidth ) { | ||
| rules.push( | ||
| `grid-template-columns: repeat(auto-fill, minmax(${ minimumColumnWidth }, 1fr))` | ||
| ); |
There was a problem hiding this comment.
Just playing around with this again, and I noticed that if we set a minimum column width that is greater than the viewport width, that the children will overflow the container:
Is it worth trying to add a guardrail to the rule so that the minimum value can never be more than 100%? Would it be possible to nest min() within minmax()? So possibly something like the following:
grid-template-columns: repeat(auto-fill, minmax(min(${ minimumColumnWidth },100%), 1fr))
That seems to work for me locally:
There was a problem hiding this comment.
Ooooh good catch! I'll have a play with it
There was a problem hiding this comment.
It worked, thanks!
There was a problem hiding this comment.
Yay, glad it was a simple fix!
|
Thanks for the feedback folks! Yeah, I wrapped both controls in a I think this is ready for another round! I'm going to go ahead and mark it ready for review, and start looking into adding/updating tests where needed. |
tellthemachines
requested review from
spacedmonkey,
ellatrix and
ajitbohra
as code owners
andrewserong
added
[Type] Enhancement
There was a problem hiding this comment.
This is looking very close to me @tellthemachines! 🎉 I left a question about handling the default / placeholder value, and what it may or may not mean for future-proofing. We've had to change default values for the Group and other blocks a couple of times, so just thought it might be worthwhile to think through what the next steps might be in a follow-up if we either wanted to change the default from 12rem or add back in the fixed number of columns.
Also left a tiny nit with spacing between the UnitControl and RangeControl. Other than that, is the last thing to get a final icon for the Grid icon / another design review?
| @@ -280,6 +280,37 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support | |||
| ); | |||
| } | |||
| } | |||
| } elseif ( 'grid' === $layout_type ) { | |||
| $minimum_column_width = ! empty( $layout['minimumColumnWidth'] ) ? $layout['minimumColumnWidth'] : '12rem'; | |||
There was a problem hiding this comment.
Just a future proofing question: if minimumColumnWidth isn't set, this assumes a default of 12rem. If we want to enable the control over individual columns again in follow-ups, how will we tell that we shouldn't be outputting 12rem?
By which I mean, what are the trade-offs between using 12rem as an inferred value, versus explicitly defining 12rem within the Grid variation? For example, what if other blocks wished to use a different default minimum column width?
There was a problem hiding this comment.
It would be pretty easy to introduce something like isResponsive again, or any other property that indicates that the grid type is not based on column width (which would always be the default). We'll need a check before outputting the value because the whole template-columns declaration will be different for a static grid, and even if a minimumColumnWidth is set (e.g. if we don't reset it on switching to another grid type) we will only use the value if the grid is of the responsive type (or whatever we want to call it).
I chose 12rem arbitrarily, it could be any other value but we do need to have some default value here or the auto-fill won't work.
If other blocks want to use a different value they can set it when they define the layout type, e.g. layout: { type: 'grid', minimumColumnWidth: '9rem' }.
There was a problem hiding this comment.
So (just to make sure I'm following), we're essentially saying, a grid layout with absolutely no other values will default to 12rem minimum column width. For future changes, we're adding additional attributes that will be set, so it'll be easy to determine when to switch off the current logic. And for changing the 12rem default on a block-by-block basis, we provide the default type type value as you mention.
Sounds solid to me! 👍
There was a problem hiding this comment.
Yeah, the idea is to have a sensible default so you can just add a grid and it'll look ok for the majority of cases? Then progressively add configurability.
| */ | ||
| function allow_grid_functions_in_styles( $allow_css, $css_test_string ) { | ||
| if ( preg_match( | ||
| '/^grid-template-columns:\s*repeat\([0-9,a-z-\s\(\)]*\)$/', |
There was a problem hiding this comment.
This looks good to me for now, but just wondering what the fix in core will be for it? I suppose we don't need to figure that out right now, but I was curious if it'll just be a matter of adding repeat to this line in core, or if it'll be more complex.
There was a problem hiding this comment.
Judging by the previous update it should be pretty straightforward.
|
I added a unit test for the new layout type ✅ Would be great to get another round of design feedback! Cc @SaxonF @jasmussen |
| // Reason to disable: the extra line breaks added by prettier mess with the unit tests. | ||
| // eslint-disable-next-line prettier/prettier | ||
| output = `${ appendSelectors( selector ) } { ${ rules.join( | ||
| '; ' | ||
| ) }; }`; |
There was a problem hiding this comment.
Another option (totally feel free to ignore this!), if we don't want to disable prettier on this line could be to perform the output over two lines. So, something like:
| // Reason to disable: the extra line breaks added by prettier mess with the unit tests. | |
| // eslint-disable-next-line prettier/prettier | |
| output = `${ appendSelectors( selector ) } { ${ rules.join( | |
| '; ' | |
| ) }; }`; | |
| output += `${ appendSelectors( selector ) } { `; | |
| output += `${ rules.join( '; ' ) }; }`; |
There was a problem hiding this comment.
I didn't want to dig too deeply into it in this PR, but it feels a bit absurd to have prettier formatting multi-line strings at all, because sometimes the spacing is there for a specific reason. Something to look at later perhaps?
There was a problem hiding this comment.
it feels a bit absurd to have prettier formatting multi-line strings
Totally, and not a blocker at all. This was just in case you didn't want the extra disable in there. I doubt we can really configure prettier here, though, as far as I know there's not a lot of configurability.
There was a problem hiding this comment.
Great work again @tellthemachines! 🎉 This is all testing nicely for me, and the default values look good, and I like that we've got a good plan for enhancements without boxing ourselves in 👍
This looks like a great place to start for the block / layout type to me. It'd be great to get another round of design feedback as you mention / possibly some more code reviews, but this LGTM! ✨
|
Nice work @tellthemachines . Feels like a solid introduction to grid with clear pathways to column count, column span etc |
What?
Fixes #47809.
Adds a new "grid" layout type, and adds it as a variation to the Group block to enable testing.
Currently the only allowed grid template is a repeating
auto-fill, with a single control to set the minimum column width.We could potentially add a variant with a fixed number of columns to create the second layout type @SaxonF shows in his demo.~~Update: I just introduced a "responsive" toggle which, when on, shows the column width control and outputs an "auto-fill" template, and when off provides a column number range and outputs a static number of responsive columns. ~~
The controls are very much for testing purposes, we'll probably want to refine this UI a bit 😅
Another thing that might be useful is adding controls to grid children to allow them to span more than one column.
Testing Instructions
Testing Instructions for Keyboard
Instructions above should work for keyboard. The block sidebar can be accessed from the block by pressing Tab twice, then tabbing through the sections until "Layout" is reached.
Screenshots or screencast