Skip to content
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

Try: Don't show a clone when dragging. #23024

Merged
merged 10 commits into from
Jun 25, 2020
Merged

Try: Don't show a clone when dragging. #23024

merged 10 commits into from
Jun 25, 2020

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Jun 9, 2020

As of #22673, the mover control is now a permanent fixture next to the block switcher, and the entire unit is a draggable handle. One thing that effort helped surface, was that the fact that we make a clone of the block when dragging feels like it breaks with some physics — you duplicate the block when you're supposed to move it. This is because you don't actually move the block until you drop it.

This PR removes the clone, and hides the block being lifted, leaving only the draggable handle. Before:

before

After:

Kapture 2020-06-22 at 15 44 57

The drag and drop experience feels slightly more precise as a result, and less muddy overall.

This description was edited as the PR changed while working on it. You can see the edit history if you like.

@jasmussen jasmussen added Needs Design Feedback Needs general design feedback. [Feature] Drag and Drop Drag and drop functionality when working with blocks labels Jun 9, 2020
@jasmussen jasmussen self-assigned this Jun 9, 2020
@github-actions
Copy link

github-actions bot commented Jun 9, 2020

Size Change: +187 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/autop/index.js 2.82 kB -1 B
build/block-directory/index.js 7.37 kB -1 B
build/block-editor/index.js 109 kB +371 B (0%)
build/block-editor/style-rtl.css 10.7 kB -161 B (1%)
build/block-editor/style.css 10.7 kB -159 B (1%)
build/block-library/index.js 130 kB +184 B (0%)
build/blocks/index.js 48.2 kB +2 B (0%)
build/components/index.js 197 kB +7 B (0%)
build/components/style-rtl.css 15.8 kB -23 B (0%)
build/components/style.css 15.7 kB -22 B (0%)
build/compose/index.js 9.62 kB -1 B
build/data/index.js 8.44 kB -1 B
build/edit-post/index.js 303 kB -3 B (0%)
build/edit-site/index.js 16.6 kB +1 B
build/edit-widgets/index.js 9.32 kB -1 B
build/editor/index.js 44.9 kB -2 B (0%)
build/keyboard-shortcuts/index.js 2.51 kB +1 B
build/list-reusable-blocks/index.js 3.13 kB -1 B
build/plugins/index.js 2.56 kB -1 B
build/rich-text/index.js 14 kB -2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 941 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-library/editor-rtl.css 7.59 kB 0 B
build/block-library/editor.css 7.6 kB 0 B
build/block-library/style-rtl.css 8.04 kB 0 B
build/block-library/style.css 8.04 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.19 kB 0 B
build/edit-navigation/index.js 9.87 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/style-rtl.css 5.5 kB 0 B
build/edit-post/style.css 5.5 kB 0 B
build/edit-site/style-rtl.css 3.02 kB 0 B
build/edit-site/style.css 3.02 kB 0 B
build/edit-widgets/style-rtl.css 2.42 kB 0 B
build/edit-widgets/style.css 2.42 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.73 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/style-rtl.css 450 B 0 B
build/list-reusable-blocks/style.css 451 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 663 B 0 B
build/nux/style.css 660 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@mtias
Copy link
Member

mtias commented Jun 9, 2020

Should the dragged block be removed? It's a bit weird to drag it but still see it in its original position.

@jasmussen
Copy link
Contributor Author

I think it should, yes, but I wonder if that's a good experience without a big refactor. I'll see what I can do with CSS, and whether that feels good.

@jasmussen
Copy link
Contributor Author

Just for quick testing, I was able to hack together this:

remove

I actually think this works rather well, even in this extremely hacky state.

Depending on feedback, we might want to do this, and use the animate component to transform the height of the block being lifted to zero so that the adjacent blocks rearrange themselves in a nice manner.

@ZebulanStanphill
Copy link
Member

That last mockup looks really nice. I think, design-wise, all it needs is a redesigned chip (pretty much exactly like the one at the bottom of the main post) and some animation so the blocks slide into place rather than instantly pop upwards when the block is removed.

@ItsJonQ
Copy link

ItsJonQ commented Jun 9, 2020

@jasmussen Whoa! I just tried this out. I do kinda miss seeing the clone, as it provides some context. But boy.. it feels much better. Way smoother. Less noise on the screen.

Also! If we were to go in this direction. Your implementation wouldn't be too off! display: none is a great way to handle this - elegant and performant.

@shaunandrews
Copy link
Contributor

I was skeptical of this based on the GIFs alone, but trying it out has me sold. I really love how simple it is, and that I no longer have a bunch of text/elements following my pointer around the screen, nor do I have multiple focus states appearing at once.

I'd love to see more work on expanding the dropzone when hovered for a second or two; Something to give me a sense of what will happen when I let go.

@draganescu
Copy link
Contributor

This is lovely! I wonder why it still shows the block in the navigation block:

Screenshot 2020-06-10 at 10 49 39

Other than that it feels very nice dragging that small symbol

@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 10, 2020

My first impulse seeing the animated gifs is that it is not enough only showing a drag icon when dragging content. I am thinking it would be nice to see the Block icon and name on dragging a block along with the drag icon.


Testing through gutenberg.run. (I will need to create a local environment, as the site is often difficult to get into.)

Drag works well.

Not working:
I was not able to drag a block below the bottom block.

A dragged block still shows a border outline even when another block is selected.
Screen Shot 2020-06-10 at 17 39 15
Selecting another block should hide the outline of the dragged block.

Suggestion 1.
Dragging a child block. Have the parent block be outlined. To help the user be able to drag a child block within the parent block.

Example.
Dragging a block inside a Group Block. Have the Group Block be outlined on drag of child block so we can see that we are staying within the boundaries of the Group Block. As it is too easy to accidentally drag a child block out of the Group Block.

I took a recording and brought it into Screenflow where I added a frame to give an idea what adding an outline around the Group block might look like while dragging child block.

Animation mixed in with a blue border mockup.

Drag-inside-Group-Block

Suggestion 2.
Make the blue border drag line thicker. Example.
Original
Blue-drop-line

Just a little thicker.
Blue-drop-line-little-thicker

@MichaelArestad
Copy link
Contributor

@jasmussen This feels solid. What is remaining for this to be wrapped up for 5.5? I liked the above idea of swapping the drag handle for the block icon and name. Is that it?

Dragging a child block. Have the parent block be outlined. To help the user be able to drag a child block within the parent block.

@paaljoachim I think that would be a good future enhancement. I think that's indicative of a larger problem beyond this PR: It's not always clear where I'm dragging when parents and children are involved. Definitely worth exploring in another issue/Pr.

@MichaelArestad MichaelArestad removed the Needs Design Feedback Needs general design feedback. label Jun 10, 2020
@jasmussen
Copy link
Contributor Author

jasmussen commented Jun 11, 2020

This feels solid. What is remaining for this to be wrapped up for 5.5? I liked the above idea of swapping the drag handle for the block icon and name. Is that it?

Thanks Michael.

What needs to happen is this:

  • components-draggable__clone needs to not be created in the first place, because it isn't necessary.
  • When the block is lifted it's currently simply hidden (display: none;) — we should (probably) use the Animate component to remove this in a way that lets adjacent blocks animate in to take the space available by the now-lifted block.
  • The dark square "chip" that shows up under your cursor when you are performing the drag operation should be a div somewhere in the markup, which shows (current thinking), Drag handle, Block type, Block name, like this more or less. Currently this dark square is a pseudo element.

I need help making those changes happen, and I've tentatively pinged @ItsJonQ for help if he has time. But I'm happy to work with anyone who has time, I know you all have so much on your plates these days.

Potentially separate & followup PRs:

  • The grabbing cursor does now show during the drag operation. Best I can tell, we have to tinker with effectAllowed for this to work, simply specifying the cursor does not work during the drag operation: https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/effectAllowed
  • To Shaun's point, we should look at expanding the dropzone when hovered for a second or two. Not so much that the layout shifts around significantly, but just enough to indicate what'll happen if you release the mouse button.

@ItsJonQ
Copy link

ItsJonQ commented Jun 11, 2020

Depending on feedback, we might want to do this, and use the animate component to transform the height of the block being lifted to zero so that the adjacent blocks rearrange themselves in a nice manner.

@jasmussen Haiii! So! I spent some time playing with this.

I have some good news and some bad news :P.

🐻 Bad news

Height animation (especially during drag/sort interactions) is really hard. We use react-spring for our animations, which is great! However, their library doesn't support height: auto handling:

Screen Shot 2020-06-11 at 1 45 05 PM

This is made more complicated by how we (currently) render Block/BlockWrapper components.

☀️ Good news

It's not impossible.

I've worked on these interactions before (from scratch even). I may be able to help here if that's the effect we want to achieve 😊

(I'm biased as I always love fluid interactions)

I have a demo of a simple case of this working:

https://d.pr/v/Wb5zHW

Some things that will be needed to make this work:

  • Size measuring functions
  • Lifecycle functions that can transition between null, 0, and clientHeight
  • An additional wrapper to handle height manipulation/overflow
  • Leveraging useSpring from react-spring
  • Potentially a custom timing function (to help it feel zippier)

I started playing around with drag/resort interactions. I don't think collapsing height is a common pattern.

It's typically one similar to Gutenberg's (clone is attached to cursor, and original stays put). Notion appears to have something similar.

Or something with a shifting placeholder dropzone, like this one:
https://clauderic.github.io/react-sortable-hoc/#/basic-configuration/basic-usage?_k=6w0vhk


No action required! Just sharing my notes :)

@jasmussen jasmussen force-pushed the try/collapse-drag-item branch from 3b0ab2c to c062474 Compare June 22, 2020 09:47
@jasmussen
Copy link
Contributor Author

How important do we feel the animations are for this one?

I looked again at this PR today, and it's trivially easy to remove the bit that clones the block. And in doing so, it fixes this issue which is rather frustrating.

The only remaining part of this PR would then be to invoke whatever is causing the surrounding blocks to shuffle around when you lift and drop a block. The same that gets called when a block gets deleted, presumably. @youknowriad, I recall you working on the block animations — do you know what causes the blocks to move around when you delete a block? And can we do that when you "lift" a block in this PR?

@youknowriad
Copy link
Contributor

@jasmussen For performance reasons, the animation triggers when the "index" of the block changes, this means if you want the animation to trigger when you start dragging, you should actually remove the block from the block list. That's a bit impactful though and may have some unexpected consequences, so I think it's wiser to try separately.

@jasmussen
Copy link
Contributor Author

jasmussen commented Jun 22, 2020

I pushed some fixes:

  • I removed the clone from the DOM. The wrapper is still there, to hold the handle being dragged.
  • I added a shadow to the dark gray "bit" indicating the dragged item.

I think with this, we could actually merge this one because it's such a better experience. However some followup items would be nice to look at, whether as fixes to this branch, or separate PRs:

  • Move the data-URI SVG for the drag handle into the icon component, and output it in the DOM instead of as a CSS background.
  • Clean up the draggable clone bits, it probably could be reduced more.
  • Explore how to add animation to the blocks when a block gets "lifted", per Riad's feedback in Try: Don't show a clone when dragging. #23024 (comment).

We should also change the handle being dragged to output the block type and name, a la: Frame 107

@jasmussen jasmussen marked this pull request as ready for review June 22, 2020 13:53
@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 23, 2020

EDIT: A comment to: #23024 (comment)

What I see is dragging a block a blue line shows up in various locations, but not the start location. The blue line should always show up as it gives a hint to where the user can drop the block (also in the default start position where drag began.) As the user might change their mind in relation to dragging a block, and need the visual hint to get it back to where it started from.

EDIT 2: Blue line signals where the user can drop a block. (if that is back at the start location makes no difference.)

@jasmussen
Copy link
Contributor Author

What I see is dragging a block a blue line shows up in various locations, but not the start location. The blue line should always show up as it gives a hint to where the user can drop the block (also in the default start position where drag began.) As the user might change their mind in relation to dragging a block, and need the visual hint to get it back to where it started fro

This was part of the reason for going with the clone concept and the blue lines in the first place, which is in contrast to other forms of drag and drop where you instantly move a block, and adjacent blocks rearrange themselves.

I used to be in the "blue line" camp for exactly the "what if I change my mind" camp, but have realized that we have big and visible undo buttons which means it's trivial to revert a change.

Your idea, showing a blue line in the spot of the block you just "lifted", is something we can try — my instinct is it simply adds confusion, because the blue line is meant to indicate dropzone, not "here's where a block used to be". There might also be a different treatment which does not indicate dropzone, that we can try. But in any case, I think it should be a followup.

@ZebulanStanphill
Copy link
Member

Behavior-wise, I think this is a definite improvement over master, and I'd like to see it get in before WP 5.5.

@jasmussen
Copy link
Contributor Author

It seems there's some consensus this is worth trying. I obviously agree, and see this path forward:

  • Merge after these fixes, try it out.
  • Explore a followup that actually removes the block from the position it's lifted, and places it where it's dropped, so that blocks rearrange themselves. This might be similar to how cut and paste works currently, CC: @mcsf if you can support that notion.
  • If the followup is complex, and if we don't like the behavior without the animation, it's trivial to unhide the block being dragged again.

label = blockType.title;
}

return <BlockDraggableChip icon={ icon } label={ label } />;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this is a separate component?

@tomtev
Copy link

tomtev commented Jun 24, 2020

Is it an idea to keep the block that is being moved and just add opacity to it? Then the content don't jump so much.
This is how we have done it with UX Builder:

Mjvco3dvNJ

@jasmussen jasmussen force-pushed the try/collapse-drag-item branch from bac44fd to 8b58379 Compare June 25, 2020 07:24
@jasmussen
Copy link
Contributor Author

I rebased and polished the chip to look like this:

handle

What I realized was that the label is not that useful, and although I didn't initially think the handle was useful, it serves well to sit under the cursor that's dragging and covering.

Per feedback in #23024 (comment), I'm unsure how to proceed.

If @youknowriad or @ellatrix have time to take a look at this one and land it, feel free to push directly to this branch.


// This is only for testing.
.is-dragging.is-selected {
display: none !important;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you try with opacity: .5 instead here? Would work much better with for example reordering columns.

Copy link
Contributor Author

@jasmussen jasmussen Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tried with opacity: 0.7.

As I see it, there are two paths forward:

  1. Go with the more radical solution of pulling the block out of the flow, literally lifting it off the page. This benefits very big blocks which take up a lot of vertical space, but can cause jumpiness.
  2. Go with the opacity change, and the chip, which is still an improvement over what we're shipping, but does not have the space saving benefits.

Honestly I think we should get option 1 in good shape, try it in the plugin, and then go with option 2 if we find after using it that the jump is jarring.

@jasmussen
Copy link
Contributor Author

Thank you for the help Ella, let's get this in and test it out, and I'll do any follow-ups based on feedback.

@mtias
Copy link
Member

mtias commented Jun 25, 2020

Agreed, let's see how it feels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Drag and Drop Drag and drop functionality when working with blocks [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.