Grid interactivity: Allow blocks to be positioned in manual mode using drag and drop #61025
Conversation
|
Size Change: +3.39 kB (+0.19%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
This is a promising start! Can we show the column/row start in the child layout panel for each block? After dragging and dropping a few blocks on an existing Grid, selecting the Grid block triggers an Also should we disable dragging into an empty grid area altogether for auto grids? It's not working too well (I tried to drop a single span block into an empty place and it turned into a 3-col block 😅) |
|
Oh, just noticed something else: we might want to add an explicit default 
|
tellthemachines
force-pushed
the
try/completely-manual-grid-mode
branch
from
006dc77
to
37ff369
Compare
|
Flaky tests detected in c80ce4d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9656919406
|
|
I've rebased and fixed a few things on this PR so it's in roughly testable form. From playing around with it, I think the biggest thing it's missing is the ability to drop into any place on a manual grid when the grid isn't selected. Currently, dropping into the grid from outside it will only show the in-between placeholder, so the item ends up falling into the first available space on the grid: grid-drop.movThe other thing that I'm not sure about is if we hijack the existing manual mode for this, how to still make available the existing manual mode. #60941 might help somewhat, but we also need to provide back compat and make sure the change isn't confusing for folks using the current grid controls. Additional feedback and ideas are very welcome here! |
Good point. I think it would make sense if
Do the in-between placeholders make sense when the grid is in manual mode? I suppose they would work okay if we implement the functionality where
Yep good flag. I think it makes sense to do that in tandem with this. |
|
OK I've updated this PR so that when grid items are moved around inside the grid, the markup is correspondingly reordered. This works whether the items are dragged and dropped, moved with the block movers or their row/column start values are updated in the sidebar. The code is pretty messy, but it seems to be working as expected. moving-blocks-around.mp4Points of friction I noticed:
Still to be addressed, probably in follow-up PRs (this changeset is already pretty big)
Any testing and feedback much appreciated! |
|
|
||
| return { | ||
| canMove: canMoveBlocks( clientIds, _rootClientId ), | ||
| rootClientId: _rootClientId, | ||
| isFirst: firstIndex === 0, | ||
| isLast: lastIndex === blockOrder.length - 1, | ||
| orientation: getBlockListSettings( _rootClientId )?.orientation, | ||
| // TODO: Doesn't feel great to couple BlockMover and grid layouts. | ||
| // TODO: Can we use useLayout() instead? | ||
| isManualGrid: layout.type === 'grid' && !! layout.columnCount, |
There was a problem hiding this comment.
Can we disable block movers here without referring to layout at all?
There was a problem hiding this comment.
Yeah, I can't really think of an alternative approach.
Maybe if we had a more flexible way of disabling moving than the existing locking APIs. Then we could have the layout block-supports call an API to lock moving. But I don't think it's worth adding that just for this feature.
Should we use useLayout though instead of reading the attributes directly? I can't remember what benefit useLayout provides, if any.
There was a problem hiding this comment.
useLayout doesn't return the correct layout type here; it only ever returns "default" in my testing. I don't think this component has access to the context from BlockListItems, which is what useLayout returns.
If it did work, the benefit would be accessing the parent layout type through context instead of a selector.
| select( blockEditorStore ).getBlockRootClientId( panelId ) | ||
| ); | ||
| const { moveBlocksToPosition } = useDispatch( blockEditorStore ); | ||
| const getBlocksBeforeCurrentCell = useGetBlocksBeforeCurrentCell( |
There was a problem hiding this comment.
Welp, had to add a bunch of hooks here to be able to move the blocks in the markup order 😅
Not sure if there's a better way of doing this. A possible alternative would be not exposing these controls in the sidebar at all (which would mean keyboard manipulation would depend exclusively on the block movers, maybe not ideal)
|
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. |
tellthemachines
added
[Type] Feature
|
Update: I've started an experiment in #62777 to track manual/auto mode with a new attribute, so we can leverage both minimumColumnWidth and columnCount to make responsive grids in either mode. |
…ell for clarity, and split up ChildLayoutControl
| const columnCountNumber = parseInt( columnCount, 10 ); | ||
| const rowStartNumber = parseInt( rowStart, 10 ); | ||
| const columnStartNumber = parseInt( columnStart, 10 ); |
There was a problem hiding this comment.
We have a lot of parseInt in this PR. Is it necessary? Can't we guarantee that the attributes are numbers?
There was a problem hiding this comment.
When the values are coming in straight from the controls they're always strings, so this has to happen at some point.
There was a problem hiding this comment.
Might be good to do an audit of this after this PR merges. Ideally we only should cast string → int once at the boundary. The less future maintainers have to think about what the data type might be, the better.
packages/block-editor/src/components/grid-visualizer/grid-visualizer.js
Outdated
Show resolved
Hide resolved
| @@ -1,2 +1,4 @@ | |||
| export { GridVisualizer } from './grid-visualizer'; | |||
| export { GridItemResizer } from './grid-item-resizer'; | |||
| export { GridItemMovers } from './grid-item-movers'; | |||
| export { useGridLayoutSync } from './use-grid-layout-sync'; | |||
There was a problem hiding this comment.
As a follow-up PR I definitely think we should rename the folder from grid-visualizer to something like grid since components like this one have nothing to do with visualisation.
There was a problem hiding this comment.
Yes! Let's do that as a follow-up in its own PR, to avoid complicating reviews of actual code changes 😅
|
This is looking good. I refactored I'd like to take one more look at We should probably also do a last round of testing. |
Co-authored-by: Robert Anderson <robert@noisysocks.com>
| __unstableDisableDropZone || | ||
| isDropZoneDisabled || | ||
| ( layout?.columnCount && window.__experimentalEnableGridInteractivity ) | ||
| ? null | ||
| : blockDropZoneRef, |
There was a problem hiding this comment.
Bit difficult to understand what's going on here. Might be good to split the logic preceding the ? into a variable e.g. isDropZoneEnabled.
|
OK I'm happy with the code 👍 Let's do a final test 😀 |
|
Only small issue I'm noticing is that this help text doesn't change when you switch between Auto and Manual mode: 
Other than that I think we're okay to merge. The new features seem relatively functional when the experiment is turned on (we'll obviously iterate) and nothing seems to be different when the experiment is turned off. |
|
Just took this for a spin, and it's looking very cool! I found one other difference when the experiment is switched off. With this PR applied, the columns input field stretches across horizontally, rather than including a range control. Is that intentional? Before 
After 
In terms of the experiment itself, I found I could sometimes unintentionally wind up with overlapping rows if I played around with moving grid items. Using the movers, I could get into a situation where the top row is fixed in the top row's position, and then I go to move the bottom row up, thinking that it'll nudge everything else to be beneath it. The result winds up being overlapping items instead of things moving out of the way. Not a blocker at all since it's part of the experiment, but just thought I'd share in case it's something to look into. It's a bit difficult to explain, so let me know if you'd like any extra info, and I'll see if I can explain it better. The following video may not be very useful, it's mostly me getting a bit confused while using the movers 😅 2024-06-25.13.09.09.mp4Definitely not a blocker to landing, once the help text is fixed up! |
| import { store as blockEditorStore } from '../../store'; | ||
| import { GridRect } from './utils'; | ||
|
|
||
| export function useGridLayoutSync( { clientId: gridClientId } ) { |
There was a problem hiding this comment.
Thinking out loud, it might make sense to trigger all of this logic from the control that toggles between Manual and Auto. That way we're not abusing useEffect.
Can change this later. Just something to think about
There was a problem hiding this comment.
Yeah that would be worth looking into!
|
Thanks for testing @andrewserong!
I don't think it's fully intentional as much as a side-effect of adding the Rows input when in Manual mode. I'm not sure if we should keep the range control when only Columns is present and remove it when we have Columns and Rows? In any case, we should probably revert the change at least when the experiment is disabled.
This is deliberate 😅 following @jasmussen's request above to allow overlapping blocks. It can be a bit disconcerting if both blocks are exactly the same size, and maybe there's something we can do visually signal there's a hidden block, but the goal is to be able to put blocks on top of other blocks in manual mode! |
noisysocks
changed the title
noisysocks
changed the title
This PR adds the ability to explicitly position a grid item by dragging and dropping it to a cell.
In order to allow more precise manipulation of the grid, when switching to manual mode all grid items are fixed in place. They can then be moved around with drag and drop, with the block movers (in any direction) or by changing the values of column and row in the Dimensions sidebar.
Positioning a grid item is done by setting the style.layout.columnStart and style.layout.rowStart attributes. These were added in #59483.
The drag and drop mechanism works by having a DropZone in each of the GridVisualizer's cells. There's some trickery here to work around the fact thatGridVisualizer needs pointer-events: none so that you can interact with the blocks that are underneath the visualiser. Pointer events are disabled on the cells until the user begins to drag a block, at which point we enable pointer events so that the drop zones work as expected.
Steps for testing:
movingblocks.mp4
Note that if a block is too big for a particular cell and there are not enough empty adjacent cells to fit it, the drop zone won't appear.
Also note that in this configuration it's possible to expand a grid item in a way that causes it to overlap with adjacent blocks (because all blocks in this mode have fixed column and row starts). This doesn't feel ideal.