Add Column Start and Row Start controls to Grid children #59483
Conversation
noisysocks
added
[Type] Enhancement
noisysocks
requested review from
tellthemachines and
ellatrix
as code owners
|
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. |
noisysocks
changed the title
|
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
|
Size Change: +98 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Positioning the items is working really well! One thing that is starting to be bothersome is that we can't reset each property individually:
My bad for not making it part of the initial implementation - it didn't seem like such an issue when all we had was Col and Row span, but it's definitely feeling off now. If you don't feel like adding a bunch of extra logic to this PR I'm happy to address it as a follow-up!
|
Yeah true @tellthemachines it would be good to split them and I think have "Grid starts" hidden by default. |
tellthemachines
force-pushed
the
add/grid-column-and-row-start
branch
from
30bf987 to
6091409
Compare
|
Just pushed a fix for an issue with columnStart in Auto mode, where once the column number goes below the start number, the grid will push the item into an auto column with no minimum size, and won't reflow gracefully: 
I adjusted the existing container query so that, if a columnStart is present, once the number of columns goes below it the item will either span full-width if it also has a columnSpan or get reset if it doesn't. I think that's the best way to deal with it but open to other suggestions! For Manual grids, should we be checking whether the columnStart is less than the columnCount? There can be unexpected effects if it goes over 😅 Had to rebase to avoid conflicts with 0b5cd48. |
Sounds good. Should we set |
I'm inlined towards setting on the input so it's clear to the users, instead of letting them set a value and then changing it. |
|
Oops forgot to |
|
I'm seeing the controls go all funny when grid is in Manual mode, can you repro this? |
I tested this and it works as you describe. I'm not sure about the behaviour though. I feel that in this case we can't possibly know what the user actually wants to do and so anything we do is a guess and will likely be unexpected. Is it useful to set a start when the grid is in Auto mode? Maybe we make it so that column start / row start can only be set when the parent layout is a Manual grid? And then clamp the start values so that
I'm not seeing this 😅 |
I think this responsive behaviour is less unexpected than what happens if we don't do anything 😅 So let's say we have an auto grid with the black-and-white cat pic pinned to the 4th col: With no responsiveness, if there's not enough space for a 4th col, the grid adds it all the same but makes it tiny: Whereas with the container query, it looks a lot more sensible even if we did have to unpin the item: I'm not super concerned with the unexpectedness because choosing an auto grid setting implies some level of giving up control on the exact position of things already 😅 so this is really just to ensure that no bad breakage occurs. Setting the grid spans to
I think it is still useful, for pinning items to the top for example. |
|
I also:
|
| // Check if columnSpan and columnStart are numbers so Math.max doesn't break. | ||
| const columnSpanNumber = columnSpan ? parseInt( columnSpan ) : null; | ||
| const columnStartNumber = columnStart | ||
| ? parseInt( columnStart ) |
There was a problem hiding this comment.
Just a drive-by comment, do we need to check that parseInt was successful (i.e. an isNaN check or something similar to how parentColumnValue is checked), or can we always trust that if it was truthy, it'll be a number? Same with the line below.
There was a problem hiding this comment.
If parseInt isn't successful it returns NaN, which will evaluate to false. Any other result should be a number 😅
There was a problem hiding this comment.
Ah, gotcha. I noticed the value winds up being used directly in the calculation for containerQueryValue so wondered if there was a chance NaN could be unexpectedly output in the container query line? Not sure if that would ever be reached in practice, though, so feel free to ignore this comment for now!
|
@tellthemachines and I were chatting and noted that one issue with letting one set Grid placement is that it doesn't update the DOM order which is not ideal for accessibility. We might need to update the block order when setting placement and set/reset the placement when re-ordering blocks using the movers. Instead of trying to anticipate what the perfect solution is I've opted to put the UI for setting Grid placement behind the Grid interactivity experimental flag so that we can move forward with #59490 and continue to iterate. |
There was a problem hiding this comment.
This is generally testing well for me, but I noticed a couple of UI issues that would be good to tweak.
- In the ToolsPanel menu,
Paddingappears between the two grid items in the menu. It'd be better for the two grid items to be next to each other.
- The styling of the input fields appears to be off as they're not filling the available area:
Otherwise testing well, and the UI appears to be safely guarded behind the experiment flag. One potential idea could be to also display the UI in the inspector controls if a value is set but the experiment is switched off, so that if folks who aren't using the experiment encounter markup that has values set, it's possible to clear out values? That might not be necessary, though, so just an idea 🙂
Oh, that's the same issue I was seeing last week! Only on the manual grid, right? But @noisysocks couldn't repro it. |
Yep, only on manual. |
| > | ||
| <InputControl | ||
| size={ '__unstable-large' } | ||
| label={ __( 'Column' ) } |
There was a problem hiding this comment.
Oh, just double-checking, should the visual label on these control also use the word placement, so that it matches the menu item?
There was a problem hiding this comment.
It would make things clearer! It's a pretty long string, though I'm not sure how relevant that is given that strings can have any length in translation.
There was a problem hiding this comment.
I don't think it's necessary. Also if we say Column placement and Row placement the labels truncate as they're too long.
I suspect we'll change the labels (and lots of other things) based on user testing.
Turns out this is webpack specific. It seems related to the https://stackoverflow.com/questions/33283901/input-number-max-attribute-resizes-field I'm not sure how to fix this at the |
I agree but I'm not sure if it's possible? It looks like |
…9483) * Separate flex child controls so each has its own reset. * Output grid-start styling * Add Column Start and Row Start controls to block inspector * Simplify php array access * Remove a few more issets * Adjust container query to take columnStart into account. * Reset grid spans together * Add max to column/row inputs * Adjust copy * Don't show Grid position by default * Use 'Grid placement' * Simplify input control labels for now * Put placement behind the Grid interactivity experimental flag for now * Fix input width issue in webkit * Show placement after span --------- Co-authored-by: tellthemachines <isabel@tellthemachines.com> Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org> Co-authored-by: noisysocks <noisysocks@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
What?
Largely follow #58539. Part of #57478.
Adds Column Start and Row Start controls to the block inspector for items that are a child of grid layout.
Also rejigs the grouping of the existing Tool Panel items so that we have Grid position and Grid span in the menu. See #59488. Props @tellthemachines.
Why?
As part of #57478 I'm working on letting the user drag and drop items within a grid to position them manually. For this to work we need to be able to set
style.layout.columnStartandstyle.layout.rowStartand we'll need a keyboard accessible alternative for setting these attributes. This PR adds both.How?
Most of the work is already done by #58539. I'm just adjusting how we output
grid-columnandgrid-rowand adding two newInputControls.Testing Instructions
Screenshots or screencast
Kapture.2024-03-01.at.10.43.28.mp4