List View: Allow dragging to all levels of the block hierarchy #49742
Conversation
andrewserong
added
[Feature] List View
|
Size Change: -242 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d7cab3c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4827537934
|
andrewserong
force-pushed
the
update/list-view-to-allow-drag-at-all-levels-of-block-hierarchy
branch
from
e712757 to
13f779c
Compare
andrewserong
force-pushed
the
update/list-view-to-allow-drag-at-all-levels-of-block-hierarchy
branch
from
e49dac0 to
752bd70
Compare
andrewserong
changed the title
andrewserong
requested review from
scruffian,
getdave,
noisysocks,
talldan,
jasmussen,
apeatling,
ramonjd and
tellthemachines
|
The code's not perfect for this one (it relies on a hard-coded value for the nesting level), but I think this PR is probably at a good place for review / feedback on the approach. The overall goal with this PR is to see if we can improve the experience for dragging through all levels of the block hierarchy. My hope is that this PR should improve things a fair bit on its own, but also unlock some of the recent discussions and ideas for how to improve drag and drop in the list view visually — the step taken in this PR is to more accurately determine which level of the block hierarchy to drag to. Happy for any feedback or ideas! In terms of scope — this PR is just about solving dragging through the different levels of the block hierarchy. I'm hoping we can look at visual changes separately (i.e. here's an exploration into changing the drag chip: #49820). |
|
Nice, just from a quick smell test, this feels substantially better and less fiddly:
Seems like a solid step forward. Nice! |
|
I couldn't get this to work at all when testing in Firefox unfortunately: Kapture.2023-05-01.at.10.26.14.mp4It works exactly as advertised when testing in Chrome though, so you must be running into some browser discrepancy. I didn't check Edge—probably a good idea to run this through Browserstack before merging. Definitely less finicky than before. I like it. More of a question for our design team and shouldn't block this PR at all but I wonder if we should display gutter lines or alternative the background colour of each leaf in the list view so that you can trace a line up with your eye to see what level you're dragging into. Will read through the code now. |
Oh, weird! It's not working in Firefox for me, either, but it is working in Safari, Edge, and Chrome. I'll dig in. Thanks for catching that! |
| @@ -52,19 +58,91 @@ import { store as blockEditorStore } from '../../store'; | |||
| * 'inside' refers to nesting as an inner block. | |||
| */ | |||
|
|
|||
| export const NESTING_LEVEL_INDENTATION = 28; | |||
There was a problem hiding this comment.
I suppose we could use getComputedStyle() to get the actual padding used. It might be slow though? Definitely benchmark it if you go down that route.
I don't really hate hardcoding this. To me, the level of indentation is a key part of what ListView communicates to the user and therefore is arguably "content". I don't think it would be inappropriate at all to set marign-left on the expander using an inline style, rewrite this logic using JavaScript, and then that way the same code that's used for setting the margin-left can be used here for calculating the drop destination.
Practically, all that we really care about, is preventing a future developer from updating the CSS I linked to above without also updating this constant. So at the very minimum, put a comment above the CSS that refers to this constant and vice versa.
There was a problem hiding this comment.
Good idea to add comments to link these together (added in 5e9068c). I also really like the idea of switching the indentation over to JS styles so that the hard-coded indentation level is used in both places 👍
If / when this PR lands, I'm happy to dig into that as a follow-up!
There was a problem hiding this comment.
I don't think it would be inappropriate at all to set marign-left on the expander using an inline style, rewrite this logic using JavaScript, and then that way the same code that's used for setting the margin-left can be used here for calculating the drop destination.
I wrote it in a similar way originally but using spacer divs (which would possibly have made this PR easier), it was changed here - #32063.
Another option could be injecting css vars for the level and indent, and then doing a calc to multiply them.
list-view/index.js also has BLOCK_LIST_ITEM_HEIGHT that could be improved in a similar way.
Sounds good for a follow-up.
There was a problem hiding this comment.
Good ideas, I'll dig into this stuff in follow-ups. Thanks!
| return point.x < blockIndentPosition; | ||
| } | ||
|
|
||
| function getDesiredRelativeParentLevel( point, rect, nestingLevel = 1 ) { |
There was a problem hiding this comment.
You have nice doc comments on all the functions except this one. Makes me feel bad for it 🥹
There was a problem hiding this comment.
I was determined to make sure these were all documented, so thanks for flagging that one, it did look lonely! Updated in ca8ec6d
|
|
||
| while ( currentBlockData ) { | ||
| candidateBlockParents.push( { ...currentBlockData } ); | ||
| currentBlockData = blocksData.find( |
There was a problem hiding this comment.
Is it worth updating getListViewDropTarget and all its helper functions to have blocksData be a Map (keyed by clientId) instead of an array so that we don't need lookups like this find()? I'm not too familiar with this code but it looks like getCandidateBlockParents will be called when the user moves the mouse so I imagine performance could be a problem if there are ~1000s of blocks.
Only bother if it makes an actual difference 😀 (premature optimisation yada yada)
There was a problem hiding this comment.
When I originally wrote it, I'm sure blockData was supposed to only be computed once when the user starts dragging. I see it's now computed on every dragover event. 🤔
Anyway, my suggestion was going to be to store the parent client ids in blocksData ahead of time, but I think the only real optimization would come from ensuring that blocksData isn't computed so often.
There was a problem hiding this comment.
I tried a fix for this in this PR - #50210
There was a problem hiding this comment.
Thanks Dan! I like that direction — just left a comment on that PR about whether we then need to add data attributes to the placeholder rows that are used as part of the windowing logic. I think that should still be pretty doable, while preserving the performance improvement?
|
Thanks again for reviewing @noisysocks — the Firefox bug took me a little longer than anticipated to debug, but I got there in the end. TIL it turns out that accessing I also like the idea for the performance improvement of keying |
It looks like in the other thread @talldan decided that the performance impact isn't significant. |
noisysocks
added
[Type] Enhancement
This is so random! How about |
Good call — I think this is preferable to relying on extra data attributes. I've swapped out the data attribute for Were there any other changes you wanted to see made before this lands @talldan? I'm happy to dig into a few code quality improvements in follow-ups (in particular I really like Rob's idea of moving to JS for the left margins so that the logic there is consolidated). |
andrewserong
added
the
[Feature] Drag and Drop
|
@andrewserong This looks good to me, thanks so much for working on this! |
|
Thanks for the reviews and feedback! Much appreciated 🙇 |
andrewserong
deleted the
update/list-view-to-allow-drag-at-all-levels-of-block-hierarchy
branch
|
I wonder if this would help at all with #46994 |
I don't think so, this PR is really just looking at how we determine the correct block drop target from within the list view based on the mouse cursor position relative to the list view items. I think #46994 would likely need to be solved for from the editor canvas drop zone side of things. Dave had a good idea over in #46994 (comment) to potential add context / metadata to the drag operation, and check this from within the editor canvas, which could be a good way to go about it. I've added a comment on that issue. |
What?
Fixes #33678
This PR seeks to add the ability to drag to all levels of the block hierarchy within the list view. This is most notable when attempting to drag beneath an expanded Group block, particularly if it is heavily nested.
Why?
It's important that folks can drag beneath a nested block. If we can allow dragging to each level of the block hierarchy, then it should allow for a more powerful drag and drop interface. Enabling this behaviour is a prerequisite for many of the current ideas for visual improvements to drag and drop — first we need to make sure it's possible to accurately determine the drop target, before we can improve how it looks.
How?
rootClientIdsblockIndexfor the dropped block based on either being at the very end of the parent (if the subsequent block is not at the same level) or by using theblockIndexof the next block (if the subsequent block is at the same level)To-do
Ensure theblockIndexis the correct one for where the user is dragging. That is, it should be the last index of the parent block or one less that the subsequent block if the drag position has the same level as the subsequent block.Ensure dragging to the root level position still works correctlyIf the block being dragged is the last of the parent's children, then currently it interferes with the check of the last visual child in the list view (as technically it isn't the last child).Fix existing tests, add additional testsrootClientId— I haven't tested this yet, but I think this should work okayTesting Instructions
Some heavily nested block markup
Screenshots or screencast