Try inserting blocks directly in empty grid cells #63108
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tellthemachines
added
[Type] Experimental
|
Size Change: +548 B (+0.03%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Size Change: +2.3 kB (+0.13%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Heck yeah, great start!
|
tellthemachines
force-pushed
the
try/inserter-in-grid
branch
from
e01d2ad
to
570f94a
Compare
|
Flaky tests detected in 570f94a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9803532009
|
|
Thanks for testing @noisysocks ! I've updated the PR so the grid visualizer only appears either when the block (or child) is selected or a block is being dragged. I've also adjusted block appender styles, but haven't got this quite right yet:
I can't seem to get rid of the plus button 😅 |
|
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. |
|
Marking this ready for review as I think it's in a pretty good place now! |
noisysocks
reviewed
Jul 8, 2024
Comment on lines
44
to
45
| ! props.attributes?.layout || | ||
| props.attributes.layout?.type !== 'grid' |
There was a problem hiding this comment.
Can be simplified to props.attributes.layout?.type !== 'grid'.
| @@ -40,6 +40,7 @@ function InserterMenu( | |||
| clientId, | |||
| isAppender, | |||
| __experimentalInsertionIndex, | |||
| __experimentalOnSelect, | |||
| onSelect, | |||
There was a problem hiding this comment.
This component has two onSelect props. It should only need one.
| @@ -150,6 +150,7 @@ class PrivateInserter extends Component { | |||
| prioritizePatterns, | |||
| onSelectOrClose, | |||
| selectBlockOnInsert, | |||
| onSelectCallback, | |||
There was a problem hiding this comment.
This component has onSelectOrClose and onSelectCallback. It should only need one.
| @@ -221,21 +318,58 @@ function GridVisualizerDropZone( { | |||
| __unstableMarkNextChangeAsNotPersistent(); | |||
| moveBlocksToPosition( | |||
| [ srcClientId ], | |||
| gridClientId, | |||
| getBlockRootClientId( srcClientId ), | |||
There was a problem hiding this comment.
We need to allow blocks from outside the grid (so with a different rootClientId) to be dropped inside the grid too.
Comment on lines
180
to
208
| onSelectCallback={ ( | ||
| block | ||
| ) => { | ||
| updateBlockAttributes( | ||
| block.clientId, | ||
| { | ||
| style: { | ||
| layout: { | ||
| columnStart: | ||
| column, | ||
| rowStart: | ||
| row, | ||
| }, | ||
| }, | ||
| } | ||
| ); | ||
| __unstableMarkNextChangeAsNotPersistent(); | ||
| moveBlocksToPosition( | ||
| [ | ||
| block.clientId, | ||
| ], | ||
| clientId, | ||
| clientId, | ||
| getNumberOfBlocksBeforeCell( | ||
| column, | ||
| row | ||
| ) | ||
| ); | ||
| } } |
There was a problem hiding this comment.
I'd move this callback logic into GridVisualizerAppender so that it's more consistent with GridVisualizerDropZone (which does all of that logic in the component) and so that this code isn't so heavily indented.
| @@ -44,6 +45,28 @@ export function GridVisualizer( { clientId, contentRef, parentLayout } ) { | |||
| ); | |||
| } | |||
|
|
|||
| const checkIfCellOccupied = ( gridElement, column, row ) => { | |||
| const cell = Array.from( gridElement.children ).find( ( child ) => { | |||
There was a problem hiding this comment.
Bit worried about performance here. We're iterating through the grid's children once per cell. So for a 16x16 grid with blocks in every cell that's 16*16*16*16 calls to getComputedCSS on every render.
An alternative approach might be to calculate rects similar to use-grid-layout-sync.js:50 and then you can tell if a cell is occupied by looking for a rect where rect.contains( row, column ).
I might try that if you don't mind me pushing to this branch.
There was a problem hiding this comment.
Sure, go ahead!
There was a problem hiding this comment.
I think the bug you're seeing with appender still appearing when a paragraph is added must happening in this function, because the gridElement refreshes with the new paragraph and its correct col and row attributes, but isCellOccupied still returns false for that cell.
There was a problem hiding this comment.
How's this look? 9a626e3
Not very scientific but it feels a bit snappier to me clicking around.
There was a problem hiding this comment.
Hmmm, I'm seeing The 'useSelect' hook returns different values when called with the same state and parameters. This can lead to unnecessary rerenders. in the console when selecting the grid.
There was a problem hiding this comment.
But the issue with the appender still rendering is fixed! And the code looks much neater
| ref | ||
| ) { | ||
| return ( | ||
| <Inserter | ||
| position="bottom center" | ||
| rootClientId={ rootClientId } | ||
| __experimentalIsQuick | ||
| onSelectCallback={ onSelectCallback } |
There was a problem hiding this comment.
Is the Callback suffix necessary? The on prefix already hints that it's a function so I'd simply call this onSelect.
|
This is cool. Makes the flow surrounding Manual mode so much more powerful now that you can start with an empty manual grid and then build up from there. Here's how everything looks: Kapture.2024-07-08.at.15.18.18.mp4We need to work on some of the design details (colours, etc.) with @jasmussen. I'm happy to merge this behind the experimental flag in a somewhat ugly state and then do a round of polish later. The minimum cell height being 8px is annoying. Collapsed cells are difficult to click on. I'm not certain how we should fix that. Do we just make collapsed cells render with a 16px minimum size? The problem is then that it wouldn't be WYSIWYG. I'm seeing one annoying bug where the appender appears on top of a newly inserted paragraph block: Kapture.2024-07-08.at.15.19.43.mp4 |
Currently the 8px minimum height is WYSIWYG because I added it to the front end too 😁 . I wouldn't object to making it slightly bigger. It seems weird to deliberately add empty rows to a grid and then not expect anything to change visually. Might be good to get feedback on this one though, there was some pushback on adding row height in #61383. I think I've addressed all the remaining feedback now! |
|
Thanks for including the summary + videos, Robert, makes it substantially easier to review.
My first instinct in seeing this is that the slivers of plus buttons are tool small to be meaningfully clickable, especially if you've limited dexterity. I realize we discussed adding a min-height of 1em (same as a default paragraph height) to those rows in the past, and it was suggested (if I'm not misremembering) that we should add as little CSS as possible. Yet those two challenges come to a head here, as without a taller min-height, clicking that plus is just not going to be a good experience. Should we add editor-only CSS? How much would this break the WYSIWYG? Some other observations:
As-is, we can't add these mover controls to the block toolbar:
Speaking of shortcuts, the existing shortcuts for moving up and down are these: We don't show those in the tooltips:
We also don't show these in the ellipsis menu, there's just a "Move to" item: Finally, these options fall apart in text-labels only mode. There's currently no way to horizontally scroll the block toolbar, and even if there was, it would be challenging for people with motor control disabilities: It suggests there's an opportunity to consolidate and improve these flows. Supposedly the DropdownMenuV2 supports nested menu items, this would allow us to do something like this quick test: 
In this, I've also explored more ergonomic keyboard shortcuts. At the moment, those particular shortcuts are used by my browser, but it seems fine in the name of simplicity to override those in the contet of the editor. None of that I would expect done in this PR. But I would avoid adding the mover controls to the block toolbar until they can be added in a way that is considerate of the accessibility problems they currently introduce around legibility and motor control. Is there a way to leverage the existing "Move to" option in the ellipsis menu for this? |
|
Thanks for the feedback @jasmussen. Agree let's try giving the rows a 1em minimum height to match the Paragraph block. I think I'd prefer that this only exists in the editor as an affordance for making the grid visualiser usable. We should be as light as possible on what frontend CSS there is for grids. Regarding the block movers, definitely didn't intend on shipping with the current ugly implementation 😅 In my mind I was hoping we could do a version of the movers where we combine the horizontal and vertical movers into one control. Something like this:
(Excuse the mockup, I'm bad at Figma.) Is that a bad idea? I don't really like the idea of burying them behind a More → Move dropdown as it's quite a central interaction to using a manual grid and our drag and drop implementation might not be foolproof in some cases. I DO really like the idea of having better keyboard shortcuts. I'm always trying to move blocks around using Cmd+Option+↑ etc. because that's what works in Bear, Notion, Craft, etc. |
Yeah I tend to agree, block movers in the manual grid are important to have handy. Could the toolbar wrap if it gets too long? That might be clunky, but it's still a better experience than not being able to access the the whole thing at all. ➕ to keyboard shortcuts! |
noisysocks
approved these changes
Jul 9, 2024
There was a problem hiding this comment.
16px/1rem still feels pretty small as a hit area to me, but I don't know what else to do. Let's ruminate on it.
And yes still a lot of UI details to work out, especially wrt to the movers. I believe it's already noted in #57478.
Still, this is good progress on an experimental feature. Onwards! 🙂
I think of the block toolbar as a forcing function. It is designed to be only the most important tools at your disposal, in the canvas, and anything else in the inspector which is designed to be the more expansive place to interact with blocks. So I would suggest, no, the block toolbar should never wrap. But this forcing function also suggests we still have work to do, to improve the block toolbar to better serve this purpose, and to be better across mobile and text-labels-only modes. Specifically for text-labels only mode, it suggests we can use more active and concise language. "Select parent" instead of "Select parent block: Grid". "Move up", instead of "Move block up" (wouldn't that need to be "Move block upwards" regardless?), "Filter", instead of "Apply duotone filter", "Add text" instead of "Add text over image", etc. |
huubl
pushed a commit
to huubl/gutenberg
that referenced
this pull request
Jul 10, 2024
Co-authored-by: tellthemachines <[email protected]> Co-authored-by: noisysocks <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: liviopv <[email protected]>
37 tasks
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
Co-authored-by: tellthemachines <[email protected]> Co-authored-by: noisysocks <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: liviopv <[email protected]>
This was referenced Jul 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #62518.
Part of #57478.
Behind the experiment flag, in manual mode:
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast