Try: Show drag slot instead of hiding the item being dragged. #33950
Conversation
jasmussen
added
the
[Feature] Drag and Drop
|
Size Change: +61 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
I think this definitely helps with the scroll content height changing. 🤔 If folks are interested in the idea, I'm guessing we might want more of a hint that it's a placeholder. The drop might be a little abrupt too without an animation, but that's probably fine. |
|
This feels much better to me. The removal of the jumping of the content on the canvas helps the eye and brain not get lost where it's looking to drag. I think the transition animations and look of the drag slot could still improve, but this is a great start. 👍 |
|
Approving changes assuming you'd iterate in followups, but happy to re-review if you continue here. |
|
Thank you for the reviews, it's definitely affirming. Because it's a somewhat bigger change, visually, I'd love thoughts by @youknowriad or @mtias. There's also a chance this change will be moot or only passing if we tackle drag and drop in a more head-on/holistic way. |
| // This hides the block being dragged. | ||
| // The effect is that the block being dragged appears to be "lifted". | ||
| // This creates a "slot" where the block you're dragging appeared. | ||
| // We use !important as one of the rules are meant to be overriden. | ||
| .is-dragging { |
There was a problem hiding this comment.
That look like a very generic selector, it could affect a lot of things (including things not rendered by core Gutenberg), should we make it more specific?
There was a problem hiding this comment.
Good point. Let me see if I can scope it better, I know that would make @gwwar happy as well 😅
There was a problem hiding this comment.
Looks like I can scope it like so: .block-editor-block-list__layout .is-dragging — would that be preferable? It doesn't feel like the right file to store that particular rule for.
The !importants still feel correct to me insofar as the way we make the "slot" appear is that we style and override the appearance of the block itself.
There was a problem hiding this comment.
For me .block-editor-block-list__block.is-dragging sounds like a better selector. Also, it seems .is-dragging class is used in the ListView as well, so we might want to check whether there's any regression there by these changes.
|
I don't have a strong opinion. If you are dragging a large section you need to scroll past it, which can be tedious. |
We could potentially apply a max-height to the "slot". There'd still be a shift like there is today, but it'd only be in cases of tall blocks. |
|
Pushed a few tweaks, and tried to apply a max-height so tall items still collapse. However a small gotcha I discovered — when you multiselect. Since the is-dragging class that we are applying is applied on a per-block basis with no wrapping container when you're multiselecting, you get this:
I suppose that's expected, but something about that feels wrong to me. I could possibly write a complex selector that would make all those separate blocks appear as one, but it seems complex. What do you think? |
|
The max-height feels a bit awkward and arbitrary to me. It might make more sense to not render it all in those cases as mentioned. |
jasmussen
force-pushed
the
try/persist-dragged-item
branch
from
8dce585 to
7aff1bb
Compare
|
Rebased and removed the max-height tweak from this one. I can't think of a simple way to make an "if taller than n pixels do this" behavior, so for the remaining issue around very tall blocks I explored these three options. 1: do nothing. Just show multiple slots:
I don't hate it. Combined with the viewport scrolling, it's technically a fairly accurate representation. 2: hide all but the first slot:
I like this one, and pushed it. It's not super accurate, but it feels like it still optimizes for the most common case. 3: hide multiselected blocks entirely:
Kind of like the current behavior, but only fur multiselected blocks. Let me know your thoughts when you have a moment. |
jasmussen
force-pushed
the
try/persist-dragged-item
branch
from
7aff1bb to
ba4eb9a
Compare
|
Rebased this one to shepard it along. As a reminder, the open question is what behavior we should apply when dragging multiple blocks. This is what the branch here does currently: That is, every block after the first one is hidden, so that if you're dragging a ton of blocks, you don't leave a very tall "slot". Coming back to this one after a while, I'm tempted to just accept that slot, and embrace this one:
What do you think? And is this still something we should land? |
There was a problem hiding this comment.
I think we should land this change by accepting that multiple blocks being dragged will show multiple slots and having no jump at all. If there's still a jump then it defeats a lot of the purpose of the change. Essentially what you have in the final gif above.
|
Cool, I'll rebase and make the change tomorrow, then we can land it. That can also be considered notice for those subscribed to the ticket! |
jasmussen
force-pushed
the
try/persist-dragged-item
branch
from
ba4eb9a to
4ab57f4
Compare
|
I've changed the behavior back to work like the following, which shows a persisted drag slot for each item:
I do like that it avoids layout shifts, that alone feels worth it to me to land this, and then we can continue looking at drag and drop improvements. For example the autoscroll behavior still feels too aggressive to me. If you feel like apprpoving the PR, feel free to also merge it when you get the chance! |
jasmussen
dismissed
apeatling’s stale review
There was a past approval, and the changes requested have been addressed.
|
Thanks, I'll land this, and I'm easy to find. There's so little code we can always rewind the clock if something happens! |
Description
This is only an experiment.
At the moment, when you drag and drop blocks, the block you're dragging is "lifted" off the page, removed from its original spot, until dropped in its new spot. Like so:
This PR experimentally changes the behavior to keep a slot where you drag from:
This is an experiment based on feedback that the current behavior is a little imprecise and prone to error. From this experiment, I feel primarily two take-aways:
In lieu of a bigger refactor, this could be an interim fix provided it's believed to be better. I don't necessarily think it is, but since it's easy to try, I thought it worth it.
Checklist:
*.native.jsfiles for terms that need renaming or removal).