Fix resizing items to top and left with GridItemResizer #60986
Conversation
tellthemachines
added
[Type] Enhancement
|
Size Change: +2.52 kB (+0.14%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
tellthemachines
requested review from
noisysocks,
richtabor,
ramonjd,
andrewserong and
fabiankaegy
|
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. |
|
Hmm, tricky. Would be good to get some design input. @jasmussen: Is there anyone in particular I should ping for grid design work while @SaxonF is away? 😀 I am thinking it probably doesn't make sense to have resizing to the top and left unless the item can also be positioned manually. |
Saxon is hard to replace, since he was so close to this grid work. I'll give this a quick shot now, and I'm always happy to be pinged. But also in general, the easiest is to ping @WordPress/gutenberg-design, that'll ping a bunch of useful folks that can likely provide good input. |
|
Taking this branch for a spin, first here's a quick GIF of trunk with the "grid interactivity" experiment enabled:
Here there are only resize handles bottom and right. Here's a GIF of this branch, description to follow: Shown here, resize handles in all four corners on select. Resizing is essentially the canvas counterpart to the column-span and row-span that exists as inspector controls. First off, this continues to be impressive and amazing work. Grid is a super powerful tool, and even if the spec itself comes with some quirks and oddities, this will ultimately be a great tool in the hands of designers, and for a broader audience it will enable fantastic patterns that are perhaps contentOnly locked, etc. So I'd love to see something land for 6.6, and that could be this resizing, if polished right. I'm still seeing an issue where occasionally the resizing will "stick", almost as if the onmouseup event doesn't register and the resizablebox just sticks to me. This is commonly when I try to break things, e.g. resize the top left cell leftwards or upwards, which obviously doesn't work. This is most easily reproducible by dragging downwards in a setup like this: You'll see that as soon as I drag outside of the grid itself, the cursor changes, and now the box sticks. In that vein, would it be sensible to hide the resize handles in the directions where resizing doesn't make sense? I.e. hide the left handle on the first column, hide the right handle on the last column, hide the top handle on the first row, hide the bottom handle on the last row? In the case of resizing downwards on the last row, that actually creates new rows. The spec allows this, and you could certainly force this by typing in a large number in the inspector. But it's not clear that there's value in optimizing the canvas controls for this, I'd be tempted to add the guardrails here, and hide the bottom handle on the last row, if possible. Probably not a blocker for launch, but something to note regardless — when resizing, it can be hard to see when you've dragged long enough for the box to "snap" and create a new colspan or rowspan. If there was some way to visualize this it would be great. Perhaps that's showing a semi-transparent version of the block with its new col/row-span until you release the cursor. I mentioned in the intro that in general the spec allows for some weird configurations. We shouldn't jump through hoops to prevent this from being possible, but if some easy heuristic comes to mind, it might be good. For example, in this GIF, scaling item 8 in a 9-grid upwards makes it a 12 grid where item 8 spans row 3 and 4:
Finally, I wonder if you had a chance to discuss in the past an alternative to the dashed outline indicating cells. They work as they are, but I wonder if we can do something neater than dashed lines. They can work, but there may be clearer options. A couple of quick tests: 
Those both work because they use That essentially ensures contrast against any background color, as it would inherit the cell-indication color. Here's another colored example: 
But in summary:
Those are mainly thoughts, let me know if they're useful, and they definitely go beyond just this PR, which probably can land if we can decide on whether to always show all 4 handles, or only do that for inner cells. |
|
Thanks for the thorough feedback and testing @jasmussen!
This should be fixed now. The resizer shouldn't stick anymore, but I also experimented with removing the resizers from the edges. Let me know if that improves things!
This is a tough one. Rearranging a grid to have less empty space in it would involve either deliberately positioning items with a column/row start, or adding something like Replacing the dashed cell indication with opacity should be easy to do. Rob's tried something like that out already in #61025, but given that one's extremely unlikely to ship in 6.6 at this point, perhaps we can move it out. Either way I think it's a good candidate for its own PR, to try and keep this one (relatively) simple for review 😅 |
|
Nice work. This is feeling very much better than trunk, and rapidly approaching something very solid. I would appreciate reviews from as many others as you can, especially folks familiar with grid, so we can approve this one. The stickiness seems addressed, and the contextual resizing handles feels like it's working well, and as intended. All of that, captured in this GIF below:
I did encounter one curiosity in that GIF above, hard to describe, but I resized a row upwards, and the first couple of resizes didn't take, and the third one seemed to suddenly create a bunch more rows than I was expecting. It's not clear it was a bug or not. And snapping, though likely very tricky to get right, may help here. |
I can reproduce this locally, pretty sure it's a bug 😅 |
This is indeed a bug. The problem is that in certain grid configurations, increasing the number of rows on an element at the edges of the grid won't change its overall size. The GridVisualizer component is observing window resizing in order to know when to make changes to the grid, but because no block dimensions change, there's no way of observing an extra row getting added. This could potentially be fixed by #61383, which by making sure auto rows always have a minimum height, will almost always trigger some change to the grid dimensions. I'm open to other ideas though! |
| bottom: 'flex-start', | ||
| }; | ||
|
|
||
| const styles = { |
There was a problem hiding this comment.
What do you think about doing this with a CSS classname instead of a styles object? BlockPopoverCover could have a .block-popover-cover and .block-popover-cover__cover class and .block-editor-grid-item-resizer could have .is-right-justified and .is-top-aligned variations.
There was a problem hiding this comment.
I mean we could do that but is there any advantage to it? Using styles is more consistent with how the component works, like with the width and height props.
There was a problem hiding this comment.
It lets us avoid adding the new additionalStyles prop. Not a big deal since BlockPopoverCover is not a public component but nice to keep API surface area low where possible. Up to you.
There was a problem hiding this comment.
Hmm yeah. I think the additional prop is a worthwhile trade-off for better legibility. Also the inline style means it's less likely some over-specific CSS will break this at any point 😄
| * The mouseup event on the resize handle doesn't trigger if the mouse | ||
| * isn't directly above the handle, so we try to detect if it happens | ||
| * outside the grid and dispatch a mouseup event on the handle. | ||
| */ |
There was a problem hiding this comment.
Should this fix live in ResizableBox so that future users of the component don't run into the same issue?
There was a problem hiding this comment.
Hmm good question. This issue only surfaces when we set bounds on the box, so it's not really useful for all the instances. And this fix depends on knowing about a parent element, which ResizableBox doesn't. It doesn't even implement its own onResizeStart, just passes it directly to Resizable. So I'm not sure it's worth adding complexity to the component for what so far is a pretty niche use case. Maybe worth doing it if similar instances pop up in other places?
There was a problem hiding this comment.
The mouseup handler could be added to ownerWindow or ownerDocument to avoid it having to know about a parent element.
But yeah, not sure if it's needed in ResizableBox, up to you.
There was a problem hiding this comment.
Hmmm not sure that'll work if we have the iframe in the way. The parent element I added the event listener to is in the actual editor canvas, but the component isn't.
packages/block-editor/src/components/grid-visualizer/grid-item-resizer.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/grid-visualizer/grid-item-resizer.js
Show resolved
Hide resolved
Maybe I don't think we can use a |
Yeah looks like there's no observer that can be used in this context. I think we can leave this issue for a separate PR though, because it's an existing one (can be reproduced on trunk by changing the Row count for a top or bottom block in the sidebar control). If we do decide to ship #61383, it'll be solved, otherwise I'm sure we'll think of something. |
tellthemachines
force-pushed
the
fix/grid-item-resizing
branch
from
48ae9b0
to
19e8ae2
Compare
|
The handles don't show/hide when the position of the element changes because of a resize: Kapture.2024-05-10.at.14.48.40.mp4 |
| * outside the grid and dispatch a mouseup event on the handle. | ||
| */ | ||
| const rootElementParent = rootBlockElement.parentElement; | ||
| rootElementParent.addEventListener( |
There was a problem hiding this comment.
We need to remove this event listener somewhere to avoid memory leaks.
packages/block-editor/src/components/grid-visualizer/grid-item-resizer.js
Outdated
Show resolved
Hide resolved
What?
Part of #57478.
Updates the GridItemResizer to allow resizing by dragging the block to the top and left. Because the resizing doesn't position the block on a specific column/row, most often resizing to the top or left will result in the block growing to the bottom or right, as it keeps its auto-placement in the grid.
I'm not sure we should meddle with auto-placement in auto mode, but in manual mode there's scope to possibly add a column/row start value to a resized block so it sticks to the area the resizer has been dragged to. I think that given the ongoing work around positioning in manual mode (see #61025) this PR could be merged as it stands (pending review of course) and placement worked on as a follow-up.
Feedback on all this is very welcome!
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
resizing-left.mov