-
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
ListView: Try showing blocks that are dragged (no longer hide them) #51724
Conversation
Could work. |
Size Change: +9 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
Obviously I'm biased but this makes dragging/dropping in list view feel much easier and more predictable. One detail we might consider is removing the dark 'chip' that appears on drag. I may be mistaken, but I think this was added as a way to keep track of which block you've picked up. But since it no longer disappears on drag, perhaps it's not necessary? |
I still like having the chip, as you can drag it onto the edit area and drop the block elsewhere. It's still nice to have that little reference. |
I think this works alright. Probably don't need the opacity applied when dragging though; it's a bit smoother without. cc @WordPress/gutenberg-design With opacity (current PR):CleanShot.2023-06-21.at.12.20.49.mp4Without opacity:CleanShot.2023-06-21.at.12.23.58.mp4 |
I think this is an improvement, although could obviously be further iterated on. The jump when you drag something out right now is disorienting, especially with a long list where you need to drag up or down further. |
Thanks for trying this out! I tested and works as described. Opacity is, I think, GPU accelerated and probably the better choice but I did notice a slight lag (nanoseconds on large lists). Chrome says its repainting at regular intervals, but it's pretty miniscule. For large lists, the opacity effect doesn't make much of a difference because when I scroll up and down I don't see the target element anyway. |
Update: I've tried out removing the opacity style, and also fixed a subtle issue where hover styles were appearing erroneously while dragging blocks. This appears to be an issue in Chrome where the hover state isn't updating while dragging the list view — the workaround is to hide/override the hover styles while dragging, which seems to be consistent with feedback on #49563 (comment) from @richtabor about removing the accent color. Here's a before / after of the hover color to show what I mean. Note how in the before GIF, the blue accent color appears to be stuck over the top block while dragging down the list. In the after GIF, there's no stuck accent color:
I think this is probably in a good state for a final review. If any designers are happy with it / want to give it an approving review, I think we should just have enough time to merge this one in for 6.3 before the GB 16.1 RC gets pushed tomorrow, if I merge this first thing Friday my time. If anyone has any concerns about the PR, though, I'm happy to skip it for 6.3 and we can do more rounds of list view improvements targeting 6.4 instead. Thanks again for all the reviews and testing! 🙇 |
Flaky tests detected in c862187a691c460f445d7192adcd02ec116cf640. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5351688922
|
d27ef7a
to
48b64e1
Compare
I think this is a good improvement, and makes it more closely match how dragging works in the canvas. I personally quite liked the opacity 😄 One part that seems confusing is that the drop indicator makes it seem like you can drop a parent as a child of itself: Kapture.2023-06-22.at.12.47.13.mp4 |
Same... happy to go with whatever the consensus winds up being, though!
Oh, good catch. I see in the |
Another update: I've added the After a little more digging, I think re-instating the 2023-06-22.15.36.53.mp4Some thoughts:
As always, happy for any feedback, but I think the current shape of this PR could make for a decent next step for the drag behaviour. |
I think that's a good observation, and I agree. Ideally we'd still iterate (probably makes sense to just hide the indicator and set |
I still think opacity isn't needed, perhaps even a bit distracting. Figma, a number of MacOS experiences maintain the selected state until moved (no opacity change).
A valid concern, but opacity shifting doesn't resolve this. Not showing the drop indicator in a dragged item would prevent the UI from indicating you can drop it in itself. |
I softly lean towards no opacity change. |
I think it's a good change with or without the opacity. 😎 |
Another update: I've come up with a fairly simple fix for the issue of dragging within nested blocks by looking up whether or not the 2023-06-23.10.26.39.mp4In this screengrab, the whole area of dragged blocks is effectively treated like it's one big list view item, and the flicking of the drop indicator to the top and bottom is supported by the dragged blocks being visually indicated that they're the ones being dragged (via opacity). While this is different behaviour to file systems like MacOS, there's some similarity with dragging items in Github projects, where you're always dragging selected tiles, so they're highlighted when they're dragged. With the list view, we have a bit of a challenge because of the difference between selected blocks and dragged blocks, which is conceptually different to both file systems and things like Github projects. For that reason, I think it's okay that what happens in the List View differs a little from interfaces in other products. If there's still time to land this PR prior to the feature freeze for 6.3, I think I'd lean toward merging in its current shape, even if we don't have agreement over whether or not to keep the opacity rule. Thanks again for all the feedback and input, everyone! |
I’m for getting it in, minus the opacity. It just has a couple unrefined edges (the white border on the drop indicator shows awkwardly alongside the opacity, the contrast ratio isn’t great, and we don’t have this opacity style applied like this elsewhere in the UI). Perhaps we can explore alternatives in follow-up PRs, but it seems less solid with the opacity. This is a nice addition overall though. |
Now that I've fixed up where the drop indicator goes when we're dealing with dragging to nested blocks, I'm happy to disagree and commit here and remove the I'll push a removal for it. Thanks again! |
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.
Nicely done. 🤜🤛
c862187
to
9339445
Compare
…ordPress#51724) * ListView: Try showing blocks that are dragged, but with lower opacity * Fix hover styling issue * Ensure we only override hover styles * Ensure the hover override only affects non-selected blocks * Re-instate opacity rule * Fix issue with drop indicator in nested dragged blocks * Remove opacity rule
What?
This PR explores an idea from @jameskoster in #49563 (comment) to try displaying blocks in the list view that are dragged.
❓ Note: this is an experimental PR exploring an idea — I'm not too sure what I think of it all up, so happy to either pursue it or close it, either way 😃
Why?
To see whether this results in less of a jump or friction while using the lift view drag and drop.
How?
display: none
rule for dragged blocks in the list viewTesting Instructions
With some nested blocks in a post or template, try dragging around some expanded blocks. You should see that the block you're dragging is no longer hidden.
How does it feel? Especially when dragging a big selection — is it easier, or harder with the blocks displayed?
Note: if we like this approach, we might need to adjust some styling in other places the list view is used (e.g. in Navigation in browse mode, where currently the opacity rule doesn't change much)
Screenshots or screencast
Here's a screengrab of a bit of drag and dropping to see how it might feel:
2023-06-21.17.02.55.mp4