Add a Row control to grid layout in manual mode. #60652
Merged
+112
−47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tellthemachines
added
[Type] Enhancement
tellthemachines
requested review from
noisysocks,
SaxonF,
ramonjd and
andrewserong
|
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: +2.8 kB (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Cool idea!
It looks like this always exposes the control for row count. Should it be opt-in per block / in block.json? With the Post Template block within the Query Loop, it now shows a Row control, but the number of items there is dictated by the query, rather than layout, so the control might not behave as expected:
Great question! I'm tempted to say that controlling rows isn't too useful unless the block also opts in to Side note: it's now becoming clear that |
|
Hooking into an existing setting where it's implicit sounds good 👍 One question about |
|
Flaky tests detected in e703092. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8657157478
|
I don't think we can do that with our current approach to dynamic blocks, because there's no way of setting attributes on each child individually, and I imagine that would be the use case: making one specific post stand out in the grid for example. So if we did want to make that possible we'd have to somehow work around the fact that children of dynamic blocks aren't blocks themselves. |
|
Ah, gotcha. Thanks for clarifying! |
noisysocks
approved these changes
Apr 15, 2024
| @@ -227,53 +239,99 @@ function GridLayoutMinimumWidthControl( { layout, onChange } ) { | |||
| } | |||
|
|
|||
| // Enables setting number of grid columns | |||
| function GridLayoutColumnsControl( { layout, onChange } ) { | |||
| const { columnCount = 3 } = layout; | |||
| function GridLayoutColumnsControl( { | |||
There was a problem hiding this comment.
Can we rename this? It's no longer just columns.
There was a problem hiding this comment.
I just had one more question after re-testing: does it matter that if the user doesn't have the interactivity experiment enabled, they can set the number of rows, but can't see the rows in the editor canvas? It doesn't really feel like a blocker to me, especially if we know we're always going to want to support explicit row values, but just thought I'd mention it in case the feature is meant to be paired with the on-canvas UI:
With experiment off
With experiment on
To put the question slightly differently: should configuring the number of rows be part of the interactivity experiment?
Oh good point, maybe it should for now! I probably wouldn't remove the sliders yet because we do have cases when the Row control isn't visible, e.g. Post Template. Let's see how this all evolves. |
|
Thanks for reviewing folks! I've addressed the feedback. |
andrewserong
approved these changes
Apr 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Part of #57478.
Adds a "Row" control, identical to the existing "Column" one. Setting a value in Row adds
grid-template-rows: repeat([row number], minmax(0, 1fr));to the grid styles.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast