Add support for column and row span in grid children. #58539
Conversation
tellthemachines
added
[Type] Enhancement
tellthemachines
requested review from
SaxonF,
jasmussen,
apeatling,
richtabor,
ramonjd and
andrewserong
tellthemachines
requested review from
spacedmonkey and
ellatrix
as code owners
tellthemachines
added
Needs Dev Note
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/layout.php ❔ lib/experimental/kses.php ❔ phpunit/block-supports/layout-test.php |
|
Size Change: +369 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I'm having a lot of fun with this so far! It's working great for me in Chrome/FF/Safari.
I don't know if it's in scope, or even a good idea, but I was wondering about a stack option for mobile, similar to what the Columns block does. Should Gutenberg offer this for grid too, or is it rather a theme responsibility?
|
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. |
|
Thanks for testing @ramonjd !
If you're using the default Grid block setting ( |
Oh thanks for explaining. I was comparing things directly with the column stack toggle. I was fooling around with fairly elaborate grids whose children had individual col/row span values. I suppose I was just musing about whether 2024-02-02.10.43.08.mp4Not a biggie, just curious. |
I guess there could be a toggle like in the Columns block, but whether there should be or how it should work is part of the wider conversation about media queries and intrinsic design. In any case I'd leave that for a subsequent iteration 😅 |
There was a problem hiding this comment.
Very cool exploration! It feels really nice being able to set a block to span multiple columns 👍
One known limitation of this approach combined with the auto columns behaviour is that, if we define a span of 3 columns for a grid child and shrink the screen down to a size where less than 3 columns total fit, the grid will keep the 3 columns and overflow the viewport if needed.
In testing this feels like it could be a bit of a blocker to me, as I could imagine folks reaching for column span and not realising that they've broken their layout for smaller viewport sizes. Or were you imagining that we could land this feature, still hidden behind the Grid feature flag, so that we're not exposing it to users until we've got an intrinsic design / responsive logic in place? If so, that could work! It'd give a bit more flexibility if we can continue to make changes behind the feature flag just in case the rules need to change to support future responsive behaviour 🤔
Here's how the smaller viewport size looked for me when using col span of 3:
Also, for some reason the column span didn't appear to be output on the site frontend for me. Here's the test markup I was using:
Some block markup with a paragraph spanning 3 cols within a Group block
<!-- wp:group {"layout":{"type":"grid"}} -->
<div class="wp-block-group"><!-- wp:paragraph {"backgroundColor":"accent-3"} -->
<p class="has-accent-3-background-color has-background">A paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"backgroundColor":"accent-4"} -->
<p class="has-accent-4-background-color has-background">Another paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"backgroundColor":"contrast-3"} -->
<p class="has-contrast-3-background-color has-background">Another paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"backgroundColor":"accent-4"} -->
<p class="has-accent-4-background-color has-background">Yet another one</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"style":{"layout":{"columnSpan":"3"}},"backgroundColor":"accent-3"} -->
<p class="has-accent-3-background-color has-background">Another paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"backgroundColor":"accent-2"} -->
<p class="has-accent-2-background-color has-background">And another one still</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->| Editor | Site frontend |
|---|---|
This is a really exciting feature, though!
lib/block-supports/layout.php
Outdated
| @@ -557,46 +557,48 @@ function gutenberg_incremental_id_per_prefix( $prefix = '' ) { | |||
| function gutenberg_render_layout_support_flag( $block_content, $block ) { | |||
| $block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] ); | |||
| $block_supports_layout = block_has_support( $block_type, array( 'layout' ), false ) || block_has_support( $block_type, array( '__experimentalLayout' ), false ); | |||
| $layout_from_parent = $block['attrs']['style']['layout']['selfStretch'] ?? null; | |||
| // If there is any value in style -> layout, the block has a child layout. | |||
| $layout_from_parent = $block['attrs']['style']['layout'] ?? null; | |||
There was a problem hiding this comment.
Tiny nit re: naming: I know the variable wasn't introduced in this PR, but $layout_from_parent makes me think of extracting the layout attribute from the parent. Would it be worth naming this something like $child_layout_style to match $child_layout_declarations?
There was a problem hiding this comment.
Yeah that's a good point! It's a tiny change so I'll just add it here.
|
Thanks for testing @andrewserong! Should work correctly in the front end now.
It might currently be more useful for grids using For the |
|
I'll just note here that I've been using a similar concept in a custom block for a while and we quickly found that grid row & column span really needs responsove controls... We were not able to find a good way to manage it without that as grids pretty much always need to reflow on smaller screens to almost a single column and when the row span it set to be larger than 1 on mobile it causes odd behaviors... Love seeing this exploration though :) |
|
Ok folks I added in a container query based on column span and parent grid column width to reset the span value once the grid shrinks to under that number of columns. A couple of disclaimers:
I guess we could use gap but it would introduce a lot of complexity because we'd have to grab the default value from theme.json if the parent block doesn't have a specific value set. I'm also not super proud of how we're accessing the parent grid's number of columns by adding it as an attribute to the child but I couldn't find another way to get a value from the parent block in the context that Any and all feedback appreciated! |
lib/block-supports/layout.php
Outdated
| $container_query_value = $container_query_value . $parent_column_unit; | ||
|
|
||
| $child_layout_styles[] = array( | ||
| 'query' => "@container (max-width: $container_query_value )", |
There was a problem hiding this comment.
This is similar to what we were testing all those moons ago for CSS layers:
Very nice to see that you've come up with a similar approach intuitively. 🍺
I think it'd be worth digging deeper to see if we can use a container query or "better_name" to nest other CSS rules, e.g., in CSS layers or nesting itself
I'd be very keen to help!
There was a problem hiding this comment.
we were testing all those moons ago for CSS layers
To elaborate a little, much of the work relies on thinking of a way to structure the rules store, e.g., using arrays to represent nested hierarchies.
There was a problem hiding this comment.
Ok ok I think I found the generic name for what we want: at-rule
Regarding the storage structure, we can always change it later if it becomes evident that there's an advantage to representing nested hierarchies with arrays. The API wouldn't change either way; that's an implementation detail. The important thing now is making sure we have the right API.
There was a problem hiding this comment.
Ok ok I think I found the generic name for what we want: at-rule
☺️
Yes! Love it.
The important thing now is making sure we have the right API.
💯
lib/block-supports/layout.php
Outdated
| /** | ||
| * If column span is set, it should be removed on small grids. | ||
| */ | ||
| if ( isset( $block['attrs']['style']['layout']['columnSpan'] ) && $block['attrs']['style']['layout']['parentColumnWidth'] ) { |
There was a problem hiding this comment.
I think we need an isset here too?
|
Ok I think that's all the feedback addressed! The only thing remaining here is to work out what to do about taking Current obstacles are:
Alternatively I can just try to use a sensible default (?) |
There was a problem hiding this comment.
Speaking in terms of the style engine changes, I really like the direction, thanks for kicking it off @tellthemachines .
I ran a few tests on the output and it handles various rules I threw at it 👍🏻
My comments are mainly brain dribble, but I think it might be good to do a very quick follow up, or maybe extract the style engine changes to another PR with tests in order to isolate the conceptual discussions and longer-term implications.
I have something going on over at:
I don't want to block progress on this PR, and I'm happy to work on an immediate follow up if time is tight, however it would be good to get feedback and ideas from @andrewserong and @aristath.
| if ( ! empty( $options['context'] ) ) { | ||
| WP_Style_Engine::store_css_rule( $options['context'], $css_rule['selector'], $css_rule['declarations'] ); | ||
| WP_Style_Engine::store_css_rule( $options['context'], $css_rule['selector'], $css_rule['declarations'], $at_rule ); |
There was a problem hiding this comment.
Disclaimer: No blockers, I'm just thinking of how we could follow up on these changes
- It would be good to bring this functionality to all style engine public API functions, e.g.,
wp_style_engine_get_stylesshould also be able to generate nested rules - The same we we allow for combining declarations on already-existing rules, we could think about how to do the same for nested containers, e.g., an "at-rule/container" already exists, any incoming rules with the same container are added to it.
| @@ -358,11 +358,11 @@ protected static function is_valid_style_value( $style_value ) { | |||
| * | |||
| * @return void. | |||
| */ | |||
| public static function store_css_rule( $store_name, $css_selector, $css_declarations ) { | |||
| public static function store_css_rule( $store_name, $css_selector, $css_declarations, $css_at_rule = '' ) { | |||
There was a problem hiding this comment.
Also for an after-the-fact-refactor:
If we create a new class of rule, e.g., a rule container with the same interface as WP_Style_Engine_CSS_Rule we might not need to change the function signature of any store methods. In that case we could let the WP_Style_Engine_Processor do the bulk of the work.
I did some experimenting with this last night:
There was a problem hiding this comment.
I'm not sure I follow. What are the advantages, and what would the API look like in that case?
There was a problem hiding this comment.
Just catching up — is the question say, what might the output look like if there's more than one rule that belongs to a particular at_rule?
So, theoretically, if we were to have an at rule for a particular query, it'd be good to group them all together into the one at_rule instead of outputting multiple?
There was a problem hiding this comment.
Sorry, my comment doesn't really make much sense. 😅 I was only thinking out loud and unfiltered of a way to minimize function signature changes.
what would the API look like in that case?
In terms of what this PR adds to wp_style_engine_get_stylesheet_from_css_rules I would change very little, if nothing. That is, passing an at_rule/container string.
The class's public methods are also part of the API, but on a lower level and the docs discourage folks from using them directly anyway, but you know... 😄
is the question say, what might the output look like if there's more than one rule that belongs to a particular at_rule?
Yeah, that could be one advantage of handling nested-rule-like objects in a separate class - it could keep track of its child rule, e.g., https://github.com/WordPress/gutenberg/pull/58797/files#diff-f23bfb067de6325fe30b9ec68f807342a3286e40b87937bcb806ea285e0252a3R205
There was a problem hiding this comment.
if we were to have an at rule for a particular query, it'd be good to group them all together into the one at_rule instead of outputting multiple
I've been thinking about this, and am not convinced there's much advantage to it. All these styles are generated on the server, and sent to the browser as part of the HTML document. Repetitive strings are well handled by gzip, so there shouldn't be much perf impact.
For these queries in particular, given that they depend on two dynamic values, there's likely to be a fair amount of variation so consolidating won't make a huge difference anyway.
On the other hand, if we did want to nest a bunch of rules under the same @rule, we'd want to make sure that it's always loaded at the very end of all the non-nested rules, because @rules (barring the special case of layers) don't affect specificity. So if we don't want to have to increase the specificity of selectors nested inside them, we need to make sure they always come after non-nested rules.
There was a problem hiding this comment.
Sure, I'm not saying we shouldn't do it at all, just that there's no particular urgency as it won't make a difference for the current use case.
For layer rules it would be great to have them grouped together. For media queries, I tend to think it's more readable to have rules that apply to the same element grouped, so the current order feels better. But I guess if we have good reason for batching (layers would definitely be that reason) then we should just do it.
Do you need that feature for this PR? I created a quick test, and I'm not sure it's following that rule
I didn't enforce it in any way, apart from making sure the rules were fed into the style engine in the correct order 😄
I'm not sure the current implementation would make that easy to enforce? In any case, it seems to work as it is so I think this would be fine for a follow-up.
There was a problem hiding this comment.
just that there's no particular urgency as it won't make a difference for the current use case.
Oh yes, all my babble has been with a quick follow up in mind.
I'm happy to take it on.
It would help in that case to have one or two unit tests to make sure any refactor doesn't break the expected output.
There was a problem hiding this comment.
...unless we want it to break expected output? In the case of consolidating rules that is. Or would that be an optional setting?
There was a problem hiding this comment.
unless we want it to break expected output? In the case of consolidating rules that is.
Oh, I'll break that 😄
A test that reflects the narrow use case of this PR's layout expectations would be great I think, e.g., building a simple nested rule. Doesn't have to be fancy. Just to have an idea of want you want to see in the output.
I think consolidation by default would work, but I'll wait for the reviews on the follow up 😄
Thanks again!
There was a problem hiding this comment.
Great discussion, folks! The idea of being able to support things like @keyframes in the future would be very cool for either a core feature or a plugin that allows folks to build custom animations. For now, though, I quite like the inclusion of the additional set_at_rule, get_at_rule, and at_rule params in this PR — it feels like stuff that likely won't box us in if we were to explore nested at rules and the like further down the track. +1 for adding a couple of quick tests to ensure we've captured the intent for now 👍
| * Otherwise, create a new entry for the at_rule | ||
| */ | ||
| if ( ! empty( $at_rule ) ) { | ||
| if ( isset( $this->css_rules[ "$at_rule $selector" ] ) ) { |
There was a problem hiding this comment.
Looks good. Would be great to have a unit test to make sure.
I think we could steal the ones over in:
| if ( empty( $this->rules[ "$at_rule $selector" ] ) ) { | ||
| $this->rules[ "$at_rule $selector" ] = new WP_Style_Engine_CSS_Rule( $selector, array(), $at_rule ); | ||
| } | ||
| return $this->rules[ "$at_rule $selector" ]; |
There was a problem hiding this comment.
Another thought for later:
The store can store any object and maybe doesn't need to care about a "$at_rule . $selector" combo.
As mentioned below if we stored something like WP_Style_Engine_CSS_Rules_Container with the same interface as WP_Style_Engine_CSS_Rule, the processor could call $rule->get_css() on all the stored "rules".
I'm just thinking out loud, but it might be easier to manipulate the entire rule set if we can quickly discern between which rules are nested and which are not. The obvious feature is nesting rules inside a single container.
|
|
||
| if ( empty( $css_declarations ) ) { | ||
| return ''; | ||
| } | ||
|
|
||
| if ( ! empty( $at_rule ) ) { | ||
| $selector = "{$at_rule}{{$suffix}{$rule_indent}{$selector}{$spacer}{{$suffix}{$css_declarations}{$suffix}{$rule_indent}}{$suffix}}"; |
There was a problem hiding this comment.
Should be consistent about prettify?
I ran a test to check spacing and indenting and see this:
+@supports (border-style: dotted){
+.gandalf {
+ color: grey;
+ height: 90px;
+ border-style: dotted;
+ align-self: safe center;
+}
+}There was a problem hiding this comment.
Sure, I wasn't really paying attention, just copied it from the other line 😅
What does {$suffix} do again?
There was a problem hiding this comment.
Good question!
I noticed last night that {$suffix} might be badly named, or is a hangover from another intention and wasn't renamed.
As far as I can tell, it means $new_line or "\n". 🤔
|
Ok folks thanks for the reviews so far! I've addressed all the feedback from the last round so I think this is ready for another round 😄 |
|
Testing well for me. The dimensions and layout panels need some general refining at some point but what do you think about moving the span controls above minimum height for now? The only other thing which is not a blocker to shipping is when you increase minimum width to a point where it pushes everything to one column, if you have an item that spans multiple columns, it should probably collapse to one column too as it throws everything off. This is just default grid behaviour, and a bit of an edge case, but something to think about in future. column-span.mp4I think we're in a decent spot and can ship once code reviews are good. This will be really useful once we have a manual setting on grid layout. |
I couldn't quite reproduce what shows in your video, could you share the markup? I did add the container query for the editor so multi-column blocks should collapse to one when the grid is single-column, but I'm not sure how well it works when resizing the grid itself, vs resizing the browser window 😅 |
lib/block-supports/layout.php
Outdated
| $container_query_value = $container_query_value . $parent_column_unit; | ||
|
|
||
| $child_layout_styles[] = array( | ||
| 'at_rule' => "@container (max-width: $container_query_value )", |
There was a problem hiding this comment.
I believe this'll need a rebase now that #58922 has landed and for this to be updated to rules_group?
There was a problem hiding this comment.
This is still testing nicely for me, and thanks for all the follow-up on the rounds of feedback! I think this looks like a good place to land it to me. There are a couple of small things that would be nice to follow-up on before Grid gets taken out of the experiment (seeing if we can infer the parent column width value, possibly tweak the scroll behaviour over the input fields for col / row span), but they don't feel like blockers to landing this initial PR to me.
LGTM! ✨
| <InputControl | ||
| size={ '__unstable-large' } | ||
| label={ __( 'Column Span' ) } | ||
| type="number" |
There was a problem hiding this comment.
Again, not a blocker for this PR, but it might be good to swap this out for NumberControl after all to hide the browser default number spinners. Otherwise with one of these fields focused, if you go to scroll the mouse, the number can scroll unexpectedly:
2024-02-14.15.12.18.mp4
Alternately we could potentially add a max value, too? In any case, not to worry about for this PR.
There was a problem hiding this comment.
Hmm I don't think hiding the spinners will remove the scrolling behaviour, at least playing with storybook it seems to persist. I'm not sure we'd want to remove it in any case? It's default input behaviour so folks will be expecting it to work.
There was a problem hiding this comment.
It's default input behaviour so folks will be expecting it to work.
Ah, fair enough! Might just be me who found it a bit confusing 🙂
| @@ -557,44 +560,95 @@ function gutenberg_incremental_id_per_prefix( $prefix = '' ) { | |||
| function gutenberg_render_layout_support_flag( $block_content, $block ) { | |||
| $block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] ); | |||
| $block_supports_layout = block_has_support( $block_type, array( 'layout' ), false ) || block_has_support( $block_type, array( '__experimentalLayout' ), false ); | |||
| $layout_from_parent = $block['attrs']['style']['layout']['selfStretch'] ?? null; | |||
| // If there is any value in style -> layout, the block has a child layout. | |||
| $child_layout = $block['attrs']['style']['layout'] ?? null; | |||
There was a problem hiding this comment.
Now whenever I see the nullish coalescing operator, all I think is "one day we'll be able to use this properly"! 😄
(This one seems fine to leave in to me since it was already there)
|
Thanks for the reviews folks! Follow-ups to come for both refactoring the parent column logic and adding an "auto/manual" toggle for grids. |
tellthemachines
added
[Type] Feature
What?
Starts addressing #57478.
Adds child layout controls for grid children. Parent blocks have to opt in to this behaviour by adding
"allowSizingOnChildren": truein theirlayoutconfig.The UI so far is just a Column span and a Row span inputs under Dimensions. They take a number, which is translated into
grid-column: span [columnSpan]. Because this is meant to work in the current auto-columns default mode of grid layout, there's no upper limit to the number of columns allowed.In the future, if we want to add the ability to pick a specific column/row to place the block, we can add, say, a
columnStartattribute and change the declaration togrid-column: [columnStart] / span [columnSpan]Testing Instructions
One known limitation of this approach combined with the auto columns behaviour is that, if we define a span of 3 columns for a grid child and shrink the screen down to a size where less than 3 columns total fit, the grid will keep the 3 columns and overflow the viewport if needed. There's no smart way of detecting the column number resizing accordingly... yet 😇
Testing Instructions for Keyboard
Screenshots or screencast