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

Add drag and drop to List View in Navigation Screen #23952

Merged
merged 45 commits into from
Sep 2, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Jul 15, 2020

Description

Addresses #22297

This PR is a work in progress technical prototype for drag and drop in Block Navigation. It's main purpose is for helping me explore how the feature can be achieved technically and collaborate on the changes. Once I'm happy with the approach I'll start extracting some of the changes into smaller PRs.

How has this been tested?

It's best to test this in the Experimental Navigation page, which has a persistent block navigation area.

@talldan talldan added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Feature] Drag and Drop Drag and drop functionality when working with blocks [Feature] List View Menu item in the top toolbar to select blocks from a list of links. labels Jul 15, 2020
@talldan talldan self-assigned this Jul 15, 2020
@github-actions
Copy link

github-actions bot commented Jul 15, 2020

Size Change: +5.27 kB (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-directory/index.js 7.99 kB +1 B
build/block-editor/index.js 128 kB +1.73 kB (1%)
build/block-editor/style-rtl.css 10.8 kB +67 B (0%)
build/block-editor/style.css 10.8 kB +65 B (0%)
build/block-library/editor-rtl.css 8.55 kB +32 B (0%)
build/block-library/editor.css 8.55 kB +31 B (0%)
build/block-library/index.js 137 kB +762 B (0%)
build/block-library/style-rtl.css 7.47 kB +23 B (0%)
build/block-library/style.css 7.47 kB +16 B (0%)
build/block-library/theme-rtl.css 741 B +12 B (1%)
build/block-library/theme.css 742 B +12 B (1%)
build/components/index.js 200 kB +720 B (0%)
build/components/style-rtl.css 15.5 kB -203 B (1%)
build/components/style.css 15.5 kB -208 B (1%)
build/data/index.js 8.55 kB +2 B (0%)
build/edit-navigation/index.js 11.6 kB -1 B
build/edit-post/index.js 305 kB +604 B (0%)
build/edit-post/style-rtl.css 6.26 kB +645 B (10%) ⚠️
build/edit-post/style.css 6.24 kB +637 B (10%) ⚠️
build/edit-site/index.js 17 kB +59 B (0%)
build/edit-widgets/index.js 12 kB +155 B (1%)
build/edit-widgets/style-rtl.css 2.46 kB +5 B (0%)
build/edit-widgets/style.css 2.45 kB +5 B (0%)
build/editor/editor-styles-rtl.css 492 B -45 B (9%)
build/editor/editor-styles.css 493 B -46 B (9%)
build/editor/index.js 45.4 kB +162 B (0%)
build/editor/style-rtl.css 3.81 kB +14 B (0%)
build/editor/style.css 3.81 kB +14 B (0%)
build/element/index.js 4.65 kB +1 B
build/i18n/index.js 3.57 kB +1 B
build/nux/index.js 3.4 kB -2 B (0%)
build/shortcode/index.js 1.7 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 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/blocks/index.js 47.7 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 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 621 B 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/token-list/index.js 1.27 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

@talldan
Copy link
Contributor Author

talldan commented Jul 29, 2020

Haven't had much time to progress this lately, but thought I'd leave a progress update:

Remaining tasks are:

  • Improve the visual drop indicators (as they were quickly hacked in using border-top at the moment)
  • There are some improvements to how we calculate where the user intends to drop. A good example is for blocks that can accept inner blocks, but currently don't have any. At the moment the user could drop a block after this kind of block or they might drop a block as a child. Usually in applications like photoshop, positioning the cursor to the right of the item allows dropping as a child, while positioning to the left allows dropping after. This is something that would be good to replicate.
  • Finesse and fix any issues (and at this point get design feedback).
  • Break up into smaller PRs

@shaunandrews
Copy link
Contributor

I posted some designs for drag-and-drop in this issue: #24029 (comment)

@talldan
Copy link
Contributor Author

talldan commented Aug 11, 2020

Thanks @shaunandrews, the designs look great and are really helpful!

One thing that would be good to clarify is how we determine when the user intends to nest a block block, versus when they'd like to drop before or after a block.

I had a look at how Figma handles this in its layers panel, and it seems like they subdivide a single Group into three drop zones, the top part for dropping before, the middle part for nesting, and the bottom part for dropping after:
figmalayerdraganddrop

In this PR currently, I've done it slightly differently. If the cursor is positioned on the left side of the block the block is dropped before/after, while to the right side of a block will result in the block being nested:
listviewdragdrop

@talldan talldan changed the title WIP: Add drag and drop to block navigation WIP: Add drag and drop to List View Aug 11, 2020
@draganescu
Copy link
Contributor

I had a quick run with this PR an I have to say it works pretty well! Great work here! This will be so useful outside of the editor as well. 🎉

@noisysocks
Copy link
Member

This is really really good. I only noticed a few quirks:

  1. You can drag an item to be adjacent to Navigation which isn't allowed. The item goes back to where it came from when you mouse up.

    one

  2. I found it a little awkward to nest an item using and drag and drop. You seem to have to move your cursor to the very right of the menu item, where I kept trying to move my cursor to the right of the insertion point.

    two

  3. Dragging to the bottom of a nested subtree doesn't seem to be possible at all. For example, here I'm trying to nest Edinburgh underneath Berlin.

    three

I forget: did we plan on having Nest and Un-nest buttons in the More options dropdown?

@talldan talldan linked an issue Aug 13, 2020 that may be closed by this pull request
@talldan
Copy link
Contributor Author

talldan commented Aug 13, 2020

Thanks for the testing!

You can drag an item to be adjacent to Navigation which isn't allowed. The item goes back to where it came from when you mouse up.

I'll need to double-check what's happening. There is a canInsertBlocks call to prevent the drop target from showing, so this shouldn't happen if there's still a template lock.

Having said that, this is how it works in the normal editor, so I don't think it's an issue if it can't be made to work.

I found it a little awkward to nest an item using and drag and drop. You seem to have to move your cursor to the very right of the menu item, where I kept trying to move my cursor to the right of the insertion point.

Yep, this is something tricky to get right. You shouldn't have to move the mouse that far to the right, but you do need to be over the block you intend to nest into (in the bottom right quadrant), it looks like your mouse was over the block below for part of the drag. That could be something to do differently.

What do you think about the comment here - #23952 (comment), would it be better replicating the way Figma handles this?

Dragging to the bottom of a nested subtree doesn't seem to be possible at all. For example, here I'm trying to nest Edinburgh underneath Berlin.

Yep, this is still TODO. Not entirely sure the best way to achieve this at the moment.

@noisysocks
Copy link
Member

What do you think about the comment here - #23952 (comment), would it be better replicating the way Figma handles this?

Hm, I think I prefer the way you did it here where hovering to the right is what indicates nesting. Only I wish it accepted hovering to the right of the insertion point as well as hovering to the right of the menu item.

@talldan
Copy link
Contributor Author

talldan commented Aug 20, 2020

@noisysocks I've now fixed issues 1 & 2 here (#23952 (comment)).

The third issue is a little more complex, looking into it next.

I also noticed dragging and dropping files doesn't seem to be working, so will debug and fix that.

@talldan talldan force-pushed the add/drag-and-drop-to-block-navigation branch 2 times, most recently from 199826c to 065ea1e Compare August 21, 2020 07:55
@talldan talldan force-pushed the add/drag-and-drop-to-block-navigation branch from 065ea1e to 0efea14 Compare August 25, 2020 04:15
@talldan
Copy link
Contributor Author

talldan commented Aug 28, 2020

@draganescu That might be best. I'd be pretty keen to follow up by working on #22113 and bring the feature back (along with the movers, more menu and appender).

edit: Now done using the existing __experimentalFeatures flag.

@shaunandrews
Copy link
Contributor

Got this working. Works well. Its kind of interesting that I can drag from the List View and then drop on the canvas and it works.

@ZebulanStanphill ZebulanStanphill added the Needs Accessibility Feedback Need input from accessibility label Aug 28, 2020
: true,
};
} );
}, [ hasPosition ] );
Copy link
Member

Choose a reason for hiding this comment

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

I think even with useEffect, we still can't re-trigger the effect when ref changed? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Opened #24914 to enable the eslint rules to help us catch these kinds of issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am definitely making an assumption that the DOM won't change during a drag/drop based on the fact that the user can't really manipulate the UI in any other way when dragging.

But at the same time, the dependency list for a hook isn't capable of handling this situation, because part of the subtree of the DOM element captured by ref could mutate, but the reference for the root element may still be the same.

For absolute correctness, a MutationObserver would probably be the best option here.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, we can make such assumption, my suggestion is more towards code smell rather than correctness. In case when we did change the DOM in the future. Hence, I don't think we should use MutationObserver at this point :)

@talldan talldan changed the title Add drag and drop to List View Add drag and drop to List View in Navigation Screen Sep 2, 2020
@talldan talldan merged commit 00f824a into master Sep 2, 2020
@talldan talldan deleted the add/drag-and-drop-to-block-navigation branch September 2, 2020 06:12
@github-actions github-actions bot added this to the Gutenberg 9.0 milestone Sep 2, 2020
@paaljoachim
Copy link
Contributor

paaljoachim commented Sep 9, 2020

I got to say I looked at the examples @noisysocks Robert shared here #23952 (comment) ,
and it sure looks really nice!
Good job! I look forward to testing this out in the 9.0.
Will the same method also be added to the List view in the regular Gutenberg screen?
I believe same method can also be used for general drag & drop in Gutenberg.

@talldan
Copy link
Contributor Author

talldan commented Sep 9, 2020

Thanks @paaljoachim. There's a PR here that unlocks some of the features in List View that were only in the navigation screen before - #25034.

It's still very early on.

Also, if drag and drop seems a bit laggy in List View, there's some optimizations I've been working on in #25069.

@SchneiderSam
Copy link

"Will the same method also be added to the List view in the regular Gutenberg screen?"

Oh yes! Pleeease!

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 [Feature] List View Menu item in the top toolbar to select blocks from a list of links. Needs Accessibility Feedback Need input from accessibility [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List View: Drag & Drop
8 participants