Try using manualPlacement attribute to set manual grid mode and allow responsive behaviour in both modes.
#62777
Conversation
tellthemachines
added
[Type] Experimental
|
Size Change: -11.9 kB (-0.68%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
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 |
tellthemachines
force-pushed
the
add/manual-placement-mode
branch
from
c5fe429
to
fe1ee8b
Compare
tellthemachines
changed the title
manualPlacement attribute to set manual grid mode.manualPlacement attribute to set manual grid mode and allow responsive behaviour in both modes.
|
I think this is in a good place functionality-wise, so marking ready for review! |
|
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. |
…tart and rowStart
|
Thanks for the updates! Not sure if it was introduced in this PR, but I notice with the experiment switched off that the range control in Manual mode isn't aligned correctly to the column count:
Also, with the experiment off and selecting a Post Template block, the help text isn't quite right. In manual mode, we're setting the number of columns but in this context manual placement doesn't apply, but the help text says "Grid items can be manually placed in any position on the grid." |
Hmm, that help text was added in #61025 and inadvertently exposed in non-experimental mode; I'll put it behind the flag again! |
This should also be fixed now! |
|
Hi! I'm a bit of a grid newbie, so I think I'm doing something wrong. Is there a difference between a null value and 0px for min column width? I'm not sure what I'm supposed be seeing when I set it to 0px using the slider vs deleting the value. Most of the video is me fumbling about, but at around 18 seconds I toggle between 0px and empty value and see different column counts. The video is on manual mode, but I get the same for Auto mode. Kapture.2024-07-01.at.15.56.42.mp4Thanks! |
Thanks for testing! that definitely shouldn't be happening, looking into it now! |
|
Ok, minimum column width behaviour should be fixed now! |
|
There's a few bugs when you try to set a manual number of rows. I'm not seeing row lines appear in the grid visualiser when I increase the number of rows: Kapture.2024-07-03.at.10.31.50.mp4The number of rows is reset to 0 (or 1, if there's inner blocks) when I re-select the grid block: Kapture.2024-07-03.at.10.35.03.mp4 |
There was a problem hiding this comment.
Nice. I really like that this clarifies that Auto / Manual are for placement and nothing else.
Should the manualPlacement attribute begin with an is prefix since it's a boolean? I can't remember if we do that for attributes or not.
I don't really like the UX around resetting column and row values. We might want to bring in ToolsPanel. Also the mix of input controls and range sliders looks cluttered to me. Something to iterate on in follow-ups.
| const { updateBlockAttributes, __unstableMarkNextChangeAsNotPersistent } = | ||
| useDispatch( blockEditorStore ); | ||
|
|
||
| const blockOrder = getBlockOrder( gridClientId ); |
There was a problem hiding this comment.
Why are we now calling getBlockOrder outside of the useSelect? If we want it to be a dep of the useEffect then it needs to be called inside the useSelect so that it updates when the store is updated.
There was a problem hiding this comment.
It doesn't seem to make a difference - on trunk, if I add a new block to a grid by pressing enter on a Paragraph block already inside it, the effect doesn't re-run. That might be due to this hook rendering inside the grid component. Ideally we get it to update whenever a grid child is added or updated, but I think that'll require bigger changes.
There was a problem hiding this comment.
I think you're right. We'll need to move <GridLayoutSync> out of toolBarControls and into a component that's not just rendered when the grid block is selected.
Regardless, I don't think this PR should be moving getBlockOrder outside of the useSelect 😀 If it were working on trunk I'd expect this change to break it.
There was a problem hiding this comment.
I've reverted this for now and will move the hook in the PR that separates the visualizer out from the grid toolbar (working branch here, still very early stages)
|
|
||
| if ( isManualGrid ) { | ||
| const rects = []; | ||
| let cellsTaken = 0; | ||
| let lowestRowEnd = 0; |
There was a problem hiding this comment.
This is a somewhat confusing name because "lowest row end" could mean the bottom-most row or the smallest "row end".
There was a problem hiding this comment.
bottomestRow? 😄
| updates[ gridClientId ] = { | ||
| layout: { | ||
| ...gridLayout, | ||
| rowCount: minimumNeededRows, |
There was a problem hiding this comment.
Is this supposed to be lowestRowEnd?
|
Thanks for reviewing @noisysocks!
This was a side-effect of addressing the feedback about uniform row height in #62928. Because I changed the default row size to Empty rows should now also be preserved. |
I'm not sure we have that many boolean attributes. The approach tends to be to deduce state from whatever attribute values are present, not to use explicit booleans. But in this case we need the boolean, so if |
|
Looks like our naming is a mixed bag. Sometimes we use https://github.com/search?q=repo%3AWordPress%2Fgutenberg%20%22type%22%3A%20%22boolean%22&type=code In testing, I am no longer seeing those two issues 👍 but now when I switch from Auto to Manual all of the grid items are put into the first cell: Kapture.2024-07-03.at.14.13.03.mp4 |
|
oooh that's what |
|
Ok I think it's working properly now:
|
|
Flaky tests detected in fe8cc84. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9772135723
|
| @@ -294,10 +335,10 @@ function GridLayoutColumnsAndRowsControl( { | |||
| onChange={ ( value ) => | |||
| onChange( { | |||
| ...layout, | |||
| columnCount: value, | |||
| columnCount: parseInt( value, 10 ), | |||
There was a problem hiding this comment.
RangeControl provides number | undefined to onChange so this parseInt is unnecessary.
There was a problem hiding this comment.
Oh I meant to do that to the NumberControl above 🤦
What?
Follow-up to #61025; addresses part of #57478; alternative to #60941.
minimumColumnWidthandcolumnCountin Auto mode. When setting both, the grid becomes responsive with a maximum column count.minimumColumnWidthas an option in Manual mode, which enables the grid to become responsive in that mode.Testing Instructions
Screenshots or screencast
Auto mode:
auto-mode.mp4
Manual mode:
manual-mode.mp4