-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Grid interactivity: Experimenting with drag and drop to set column start and row start #59490
Conversation
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: -1.12 kB (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
30bf987
to
6091409
Compare
This reverts commit bfe3042.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, the drag/drop and resize functionality is working like butter for me. Incredible work.
Since it's behind the experimental flag, take all my comments with a bag-load of Himalayan salt.
|
||
return ( | ||
<BlockPopoverCover | ||
className={ classnames( 'block-editor-grid-visualizer', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhere a little z-index
jig in mobile view is required as it sits over the sidebar
2024-03-15.12.09.46.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Why is this happening? When I look in the inspector the visualiser has z-index = 30
and the list view has z-index = 10000
😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect they're in different stacking contexts? Or a new one needs to be created around the grid visualizer.
This tool is pretty handy to work it out: https://github.com/gwwar/z-context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah they're in different stacking contexts but what's causing the issue is the popover content moving to the bottom of the DOM when the viewport is smaller than 782px:
I'm not sure what the role of components-popover__fallback-container
is in popover logic, but it appears after the sidebar and is a sibling of #wpwrap
so I suspect the only way to fix this with CSS would be to assign a non-negative z-index to #wpwrap
. I doubt that would be free of consequences though 😅
|
||
function GridVisualizerGrid( { clientId, blockElement } ) { | ||
const [ gridInfo, setGridInfo ] = useState( () => | ||
getGridInfo( blockElement ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the new kid: this is so the visualizer matches the underlying grid when resizing the window? Works really well. Nice 🤟🏻
Feel free to ignore this opinion: "Info" sounds to me like something I'd ask for at a truck stop on my way to an obscure holiday township with no mobile reception.
Just jokes. What about 'properties'? E.g., getBlockElementGridProperties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does handle window resizing but that's secondary. The primary purpose is so that we recalculate the grid lines when the grid or its children change size e.g. if the user changes the number of columns in the inspector.
I didn't want to use the words attributes or properties because I didn't want to imply that we're returning HTML attributes, React props, or CSS properties. I think a generic word like data or info is right. But idk happy to do what you say here 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good! Just a passing comment with as much value as it cost in the first place = $0
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to ask clarifying questions, it means they're answered for future readers too 🙂
packages/block-editor/src/components/grid-interactivity/grid-item-pin-toolbar-item.js
Show resolved
Hide resolved
I'm working on two issues @SaxonF identified:
|
Should we implement that before this grid work then? Also, in that example, the static blocks are visually distinguishable from the others. I'm not sure it would be so clear if they all looked exactly the same. |
Yes we'd want to distinguish between them, even if that's just when starting to drag. |
Oh, one other item worth adding to the list of follow-ups: it should be possible to drag a block from outside of the grid into a pinned position on the grid. |
This is really a drag and drop enhancement, not specific to grid. I don't think we can untangle them. I dug up #16457 which is an old exploration into doing it that was abandoned because it didn't play nicely with nested blocks. Maybe something to think about but probably not a can of worms we want to open right now. |
How should we visualise it? |
Alright I'll work on a better solution to this. My previous attempt was too naive. |
Sorry if this has been covered before - on the face of it I'm finding undo seems to work for some interactions and not others. 2024-04-18.11.45.17.mp4I think it's tied to the cell. So if I'm editing a single cell the history isn't saved. If I edit different ones it appears to work as expected 2024-04-18.11.51.48.mp4 |
@noisysocks for this comment, can you use |
… 'natural position'
I got halfway through addressing this comment in b70baee. It's pretty complicated to work out when a block needs a Instead of doing that 😅 I am thinking we should look at adding constraints. For example not allow a user to set I'm also curious about creating a strong distinction between auto mode and manual mode where, in manual mode, all grid items have a For now though @SaxonF is hounding me about wanting to see how "click or drag a cell to insert a block" feels so I'm going to work on that. Safe to say this PR is a grabbag of a few different ideas, none of which are production ready, and so I've marked it as a draft again. |
I've pushed a barely-usable first pass at this 😀 |
"hounding" 🤣 . Accurate though! Agree we should re-explore everything pinned in manual (ie no pinning). When we tried last time we didn't have rows in place plus a couple of the other improvements which affected how it felt. "click or drag a cell to insert a block" will give us a more complete experience to try. We can frame "type" as grid item position and highlight the difference between auto and manual. In auto we can even allow user to set number of columns (or auto as default) which means the only gap we'd have is being able to mix both manually placed and automatically placed items. We can look at this in future as an advanced feature. |
One other note. When dragging a grid item we should add outlines to all grid items (like when you hover over them) so its clear where their boundaries are. |
If all grid items have a
I think if we do this, it's important that the experience be consistent with other blocks, though the underlying behaviour may be different. E.g. we should make sure it's still possible to move blocks in list view, and perhaps replace the default block movers with a two-dimensional set of controls that allows to move both vertically and horizontally (this was @noisysocks' suggestion in conversation with me earlier). Ideally we want the action of moving blocks, whether by drag and drop or keyboard, to be the same as it is for all other blocks except when these blocks move within the grid, their The only unique visible behaviour is that we want to be able to drag or keyboard move one of these blocks to any empty grid cell (that might not be possible to do in list view, but it should be doable through either the block movers or the column and row start controls, apart from canvas drag and drop). Grid is a unique form of container in that its configuration determines the exact space its children occupy, and it displays empty grid cells if there aren't enough children to fill it. It's quite different from flex in this respect, because in flex the size of the child to some extent determines the distribution of space, and unless the children are few and very small, there is no extra space within the container. I'm rambling a bit 😅 but what I'm trying to get at is that grid cells are a sort of virtual container that we could treat similarly to how empty paragraphs are treated if we wanted them to have a presence in list view. We're already kind of treating them as such with the ability to drop into any grid cell. |
Iterating on the above. If we did make it so that in manual mode everything has a position. Based on this conversation an alternative would be adding max column setting on auto mode which would cover most scenarios |
Hey all 👋 This is really impressive work here 😮 The way this behaves is much smoother than I would have guessed originally. The one thing that I keep coming to here is what the responsive story is. Taking the example shown in the ticket description for example. How would one really control the order & width of various elements there without the concept of States or something similar 🤔 Would love to hear how you all are thinking about this when it comes to the grid feature. For "simple" grids the "minimum column width" serves that use-case pretty well. But I'm struggling with these more complex grids. See this example here of a grid that is a little more complex: CleanShot.2024-04-22.at.21.33.28.mp4 |
@fabiankaegy that looks like a bug in the responsive logic for grid elements spanning multiple columns. I'll look into it, thanks for flagging! Update: I have a fix in #60976. |
This PR has morphed into more of an exploration of a few different ideas. I'll split the production-ready parts out into separate PRs when ready.
What?
In #59052 I added a new Grid interactivity experiment and the ability to set the column and row span of a grid item using a resizable box.
This PR builds off of that work and adds the ability to explicitly position a grid item by dragging and dropping it to a cell.
Why?
Part of #57478.
How?
Positioning a grid item is done by setting the
style.layout.columnStart
andstyle.layout.rowStart
attributes. These are added in #59483, which this PR is stacked onto.If a grid item has a
columnStart
androwStart
then it is considered pinned. This is indicated in the block toolbar using a toggle button. Unpinning a grid item resetscolumnStart
androwStart
and will result in the grid item once again being positioned automatically by the grid container.The drag and drop mechanism works by having a
DropZone
in each of theGridVisualizer
's cells. There's some trickery here to work around the fact thatGridVisualizer
needspointer-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.Testing Instructions
Screenshots or screencast
Kapture.2024-03-01.at.14.06.56.mp4