feat: add experimental drag-drop package #11864

crisbeto · 2018-06-20T19:06:28Z

Adds an initial version of the drag-drop module to cdk-experimental with support for the following:

Dragging an element on its own.
Dragging an element using a handle.
Sorting an element within a drop zone.
Moving an element between drop zones.
Touch and mouse support.
Custom template for the preview element.
Custom template for the placeholder element.
Animation support for the preview.

Demo: https://material-cb7ec.firebaseapp.com/drag-drop

Relates to #8963.

Note: this is still missing a lot of unit tests. I'll continue adding them to this branch or open a follow-up PR. Also will continue adding features, docs and a11y guidelines in follow-ups.

josephperrott · 2018-06-21T14:40:14Z

src/cdk-experimental/drag-drop/drag-utils.ts

+ * @param fromIndex Starting index of the item.
+ * @param toIndex Index to which the item should be moved.
+ */
+export function moveItemInArray(array: any[], fromIndex: number, toIndex: number): void {


Can use a generic here instead of any for the array? I would just prefer to avoid any where possible personally, though since the typing never leaves this function, I suppose its not crucial.

moveItemInArray<T>(array: T[], fromIndex: number, toIndex: number): void {

Agreed, it would be better to have a generic

josephperrott · 2018-06-21T14:44:24Z

src/cdk-experimental/drag-drop/drag.spec.ts

+import {dispatchMouseEvent, dispatchTouchEvent} from '@angular/cdk/testing';
+import {CdkDrag} from './drag';
+import {CdkDragDrop} from './drag-events';
+import { moveItemInArray } from './drag-utils';


Remove extra spacing around symbol

josephperrott · 2018-06-21T14:59:46Z

src/cdk-experimental/drag-drop/drag.ts

+
+  constructor(
+    /** Element that the draggable is attached to. */
+    public element: ElementRef<HTMLElement>,


Is this meant to be public but not internal (_ prefixed)?

I left it public so people can use the element to add custom styling while it's being dragged.

josephperrott · 2018-06-21T15:01:22Z

src/cdk-experimental/drag-drop/drag.ts

+  ngAfterContentInit() {
+    // TODO: doesn't handle (pun intended) the handle being destroyed
+    const element = (this._handle ? this._handle.element : this.element).nativeElement;
+    element.addEventListener('mousedown', this._pointerDown);


it looks like these event listeners are not cleaned up with a removeEventListener

They're not, but presumably they live for the lifetime of the directive since they're bound either on the host or the handle which is inside the host.

josephperrott · 2018-06-21T15:06:51Z

src/cdk-experimental/drag-drop/drag.ts

+
+    if (this.dropContainer) {
+      const element = this.element.nativeElement;
+      const preview = this._preview = this._createPreviewElement();


Rather than the multiple assignment here, should _createPreviewElement also assign its output to this._preview?

It could be thought of as setupPreviewElement rather than just creating. I think this especially makes sense since _createPreviewElement already has side effects of creating the refs needed for the preview.

I guess this is more of a personal preference. I prefer not to have functions initialize properties like this since it isn't very transparent.

I agree w/ Kristiyan

josephperrott · 2018-06-21T15:30:33Z

src/cdk-experimental/drag-drop/drag.ts

+}
+
+/** Point on the page or within an element. */
+interface Point {


This same interface is created and used in flexible-connected-position-strategy.ts, perhaps it should be moved to a common place?

I decided to duplicate it since it's fairly small.

Agreed that it's trivial enough to not bother finding a common place for it, which would necessitate it being part of a public API

crisbeto · 2018-06-21T17:31:17Z

Addressed the current feedback @josephperrott.

jelbourn · 2018-06-21T17:25:33Z

src/cdk-experimental/drag-drop/drag-events.ts

+
+/** Event emitted when the user moves an item into a new drop container. */
+export interface CdkDragEnter<T> {
+  /** Container from which the user has a removed an item. */


I think this is accidentally using the same comments as CdkDragExit

jelbourn · 2018-06-21T17:27:30Z

src/cdk-experimental/drag-drop/drag-events.ts

+  /** Index of the item when it was picked up. */
+  previousIndex: number;
+  /** Current index of the item. */
+  currentIndex: number;


Do dragged items have to inherently have an index?

The drop event only fires on items inside a drop zone which ensures that they have an index.

jelbourn · 2018-06-21T17:28:39Z

src/cdk-experimental/drag-drop/drag-handle.ts

+/** Handle that can be used to drag and CdkDrag instance. */
+@Directive({
+  selector: '[cdkDragHandle]',
+  host: {


Is there a TODO here for a11y treatment?

I have it on my list, I just haven't added any extra APIs to facilitate it (e.g. we need something pick up and swap items programmatically).

jelbourn · 2018-06-21T17:30:37Z

src/cdk-experimental/drag-drop/drag-utils.ts

+ * @param fromIndex Starting index of the item.
+ * @param toIndex Index to which the item should be moved.
+ */
+export function moveItemInArray(array: any[], fromIndex: number, toIndex: number): void {


Agreed, it would be better to have a generic

jelbourn · 2018-06-21T17:37:13Z

src/cdk-experimental/drag-drop/drag.ts

+  /** Coordinates on the page at which the user picked up the element. */
+  private _pickupPositionOnPage: Point;
+
+  /** CSS `transform` applied to the element when it isn't being dragged.  */


Expand this comment to explain why there's a passive transform applied when the element isn't being dragged?

jelbourn · 2018-06-21T20:57:02Z

src/cdk-experimental/drag-drop/drop-container.ts

+  /**
+   * Drops an item into this container.
+   * @param item Item being dropped into the container.
+   * @param currentIndex Index at which the item should be inserted.


Similar question as above: could you have a dropzone without indices? E.g., you have free-floating draggables that can only be placed within certain regions?

I'm not sure that makes sense. If you wanted a region with a bunch of freely-draggable items, you'd just place them in something that isn't a CdkDrop.

I'm thinking about free-drag with constraints. I see those constraints as having two manifestations: blocking you from moving into/out of some element entirely, or "cancelling" a drop if it's in a disallowed area. I do think it is reasonable for that to be done with a different directive than cdkDrop

Yeah I was thinking either a directive or a straight-up @Input that accepts an ElementRef, although that could be problematic for components with an implicit exportAs.

jelbourn · 2018-06-21T20:57:36Z

src/cdk-experimental/drag-drop/drop-container.ts

+import {CdkDrag} from './drag';
+
+/** @docs-private */
+export interface CdkDropContainer<T = any> {


Thoughts on DropContainer vs. DropZone or DropRegion?

I'm fine with DropZone, I just needed a name that isn't CdkDrop.

Don't worry about changing it now; I'm not strongly attached to any of them. I'll sleep on it.

jelbourn · 2018-06-21T21:01:51Z

src/cdk-experimental/drag-drop/drag.ts

+  @ContentChild(CdkDragPlaceholder) _placeholderTemplate: CdkDragPlaceholder;
+
+  /** Emits when the user starts dragging the item. */
+  @Output() started = new EventEmitter<CdkDragStart>();


@Output('cdkDragStarted') started and similar for other outputs since this is an attribute selector directive

jelbourn · 2018-06-21T21:03:16Z

src/cdk-experimental/drag-drop/drop.ts

+   * Other draggable containers that this container is connected
+   * to and into which the container's items can be transferred.
+   */
+  @Input('connectedTo') siblings: CdkDrop[] = [];


I think it's a little confusing for the input and the property to be so different

jelbourn · 2018-06-21T21:06:14Z

src/demo-app/drag-drop/drag-drop-demo.html

@@ -0,0 +1,38 @@
+<div class="list">


Andrew has been pushing for is to stop adding demos that only live in demo-app/ and instead always embed something from the examples, since it forced us to flesh those out. Not necessary for this PR, but would be nice to do.

crisbeto · 2018-06-22T15:20:11Z

Addressed the latest set of feedback.

jelbourn

LGTM

josephperrott · 2018-06-27T16:07:40Z

@crisbeto Please rebase when you have a chance

Adds an initial version of the `drag-drop` module to `cdk-experimental` with support for the following: * Dragging an element on its own. * Dragging an element using a handle. * Sorting an element within a drop zone. * Moving an element between drop zones. * Touch and mouse support. * Custom template for the preview element. * Custom template for the placeholder element. * Animation support for the preview. Demo: https://material-cb7ec.firebaseapp.com/drag-drop Relates to angular#8963.

crisbeto · 2018-06-27T17:34:57Z

Rebased @josephperrott.

cengiz-dev · 2018-07-03T10:14:55Z

@josephperrott, @crisbeto, do you know when this feature will be included in a release?

angular-automatic-lock-bot · 2019-09-09T04:03:42Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

crisbeto assigned jelbourn Jun 20, 2018

crisbeto requested review from devversion and jelbourn as code owners June 20, 2018 19:06

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 20, 2018

crisbeto added the target: master only label Jun 20, 2018

crisbeto force-pushed the drag-and-drop-poc branch 4 times, most recently from 3d4aa8d to 3e401e7 Compare June 20, 2018 19:44

josephperrott self-requested a review June 20, 2018 22:36

josephperrott reviewed Jun 21, 2018

View reviewed changes

crisbeto force-pushed the drag-and-drop-poc branch from 3e401e7 to 4b5602d Compare June 21, 2018 17:31

jelbourn reviewed Jun 21, 2018

View reviewed changes

crisbeto force-pushed the drag-and-drop-poc branch from 4b5602d to 4b7b740 Compare June 22, 2018 15:19

jelbourn approved these changes Jun 22, 2018

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jun 22, 2018

jymdman mentioned this pull request Jun 24, 2018

implementing cdk drag-and-drop #11905

Closed

crisbeto force-pushed the drag-and-drop-poc branch 3 times, most recently from 3740c18 to 4a40f9a Compare June 25, 2018 21:04

josephperrott added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Jun 27, 2018

crisbeto force-pushed the drag-and-drop-poc branch from 4a40f9a to d3aaa18 Compare June 27, 2018 17:33

crisbeto added the action: merge The PR is ready for merge by the caretaker label Jun 27, 2018

crisbeto removed the pr: needs rebase label Jun 27, 2018

josephperrott merged commit 27811a3 into angular:master Jun 28, 2018

pas2al mentioned this pull request Jul 6, 2018

cdk drag and drop - cdkDragDropped event not triggered #12086

Closed

josephperrott pushed a commit that referenced this pull request Jul 9, 2018

feat: add experimental drag-drop package (#11864)

bb16abf

victoriaaa234 pushed a commit that referenced this pull request Jul 25, 2018

feat: add experimental drag-drop package (#11864)

046f9b9

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019

feat: add experimental drag-drop package #11864

feat: add experimental drag-drop package #11864

Conversation

crisbeto commented Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

crisbeto commented Jun 22, 2018

jelbourn left a comment

Choose a reason for hiding this comment

josephperrott commented Jun 27, 2018

crisbeto commented Jun 27, 2018

cengiz-dev commented Jul 3, 2018

angular-automatic-lock-bot bot commented Sep 9, 2019

crisbeto commented Jun 20, 2018 •

edited

Loading

crisbeto Jun 21, 2018 •

edited

Loading

jelbourn Jun 21, 2018 •

edited

Loading