refactor(drag-drop): move logic out of directives #14350

crisbeto · 2018-12-02T15:13:33Z

Moves the logic for CdkDrag and CdkDropList into separate, non-Angular-specific classes. This is a first step towards allowing consumers to attach drag&drop functionality to arbitrary DOM nodes.

Note: the intent of this PR is to move all of the logic out of the directives and to retain backwards compatibility. As such, none of the new classes are exposed via the public API. The next step will be to add a nicer API around DragRef and DropListRef and to make them a bit more usable.

ngbot · 2018-12-03T23:51:52Z

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

jelbourn

Is anything getting formally deprecated in this PR?

jelbourn · 2018-12-04T22:03:26Z

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

+ * @deprecated Use `DragRefConfig` instead.
+ * @breaking-change 8.0.0
+ */
+export interface CdkDragConfig extends DragRefConfig {}


Does this need to live in this file? Could it instead live in public-api.ts?

jelbourn · 2018-12-04T22:07:29Z

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

+    'class': 'cdk-drag',
+    '[class.cdk-drag-dragging]': '_dragRef.isDragging()',
+  },
+  providers: [{


Collapse this provider to one line?

jelbourn · 2018-12-04T22:15:18Z

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

+    public element: ElementRef<HTMLElement>,
+    /** Droppable container that the draggable is a part of. */
+    @Inject(CDK_DROP_LIST) @Optional() @SkipSelf()
+    public dropContainer: CdkDropList,


Could you change this directive to use DropListRef internally and have CdkDropList provide itself as DropListRef? The only place it's referenced directly is to get disabled, which also exists on DropListRef. The dropContainer would still have to be exposed publicly as deprecated, but it could eventually get rid of it.

Another change I'd like to make for CdkDrag would be to reference the idea of the more generic "drop container" rather than the specific "drop list", where the container is the lower-level thing that just knows about receiving drops and not about lists/reordering.

Regarding providing the CdkDropList as a DropListRef, I'm not sure whether it would be very useful if somebody got it through DI. Also I wanted to reduce the amount of deprecations and doing this will involve having to add another parameter to the constructor.

As for the more generic drop container, I agree, but it was something I was planning to do once we started implementing more container types.

jelbourn · 2018-12-04T22:26:29Z

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

+    @Inject(CDK_DRAG_CONFIG) private _config: DragRefConfig,
+    @Optional() private _dir: Directionality) {
+
+    const ref = this._dragRef = new DragRef(element, this._document, this._ngZone,


This is a pretty complex constructor. Could we simplify it by having a factory service that constructs the refs for you? Something like a DragDrop service with a method analogous to Overlay.create. Maybe something like dragDrop.createDraggable(...): DragRef and dropDrop.createDropContainer(...): DropContainerRef

Agreed, my plan was to add a service for it in a follow-up. The point of this PR was to get the more difficult part of the refactor out of the way so that we can focus on the public API for it afterwards.

jelbourn · 2018-12-04T22:38:37Z

src/cdk/drag-drop/directives/drop-list.ts

+let _uniqueIdCounter = 0;
+
+// tslint:disable:class-name
+// TODO: exclude from public-api.ts


This is something we'd have to do before the commit lands.

Since it won't be exported at all, we could use a name like CdkDropListInternal. It would also be good to have some tooling that could enforce that certain symbols are not exported through the public api.

Isn't this the purpose of the @internal tag? I know that we removed them to avoid downstream issues, but the symbols should be preserved if we didn't enable stripInternal.

Yeah, I have no strong feelings about which tag we use

jelbourn · 2018-12-04T22:40:23Z

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



+/**


Could you add a comment above here that explains the background on why this is being removed?

jelbourn · 2018-12-04T22:44:52Z

src/cdk/drag-drop/directives/drop-list.ts

+   * @param item Item to be sorted.
+   * @param pointerX Position of the item along the X axis.
+   * @param pointerY Position of the item along the Y axis.
+   * @param pointerDeta Direction in which the pointer is moving along each axis.


pointerDeta -> pointerDelta

jelbourn · 2018-12-04T22:48:32Z

src/cdk/drag-drop/drop-list-ref.ts

-  /** Draggable items in the container. */
-  @ContentChildren(forwardRef(() => CdkDrag)) _draggables: QueryList<CdkDrag>;
+// tslint:disable:class-name
+// TODO: exclude from public-api.ts


Same this here as in drop-list.ts

jelbourn · 2018-12-04T22:52:25Z

src/cdk/drag-drop/directives/drop-list.ts

+   * @param x Pointer position along the X axis.
+   * @param y Pointer position along the Y axis.
+   */
+  _isOverContainer(x: number, y: number): boolean {


Could this function be eliminated?

It can be, I left it in to keep the CdkDropList compatible with the CdkDropListContainer interfaces.

jelbourn · 2018-12-04T22:54:30Z

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

+
+
+// tslint:disable:class-name
+// TODO: exclude from public-api.ts


Same here as other places

crisbeto

@jelbourn the only things being formally deprecated are the ones that we were using to avoid circular imports, however they weren't public to begin with.

crisbeto · 2018-12-05T06:57:49Z

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

+    public element: ElementRef<HTMLElement>,
+    /** Droppable container that the draggable is a part of. */
+    @Inject(CDK_DROP_LIST) @Optional() @SkipSelf()
+    public dropContainer: CdkDropList,


Regarding providing the CdkDropList as a DropListRef, I'm not sure whether it would be very useful if somebody got it through DI. Also I wanted to reduce the amount of deprecations and doing this will involve having to add another parameter to the constructor.

As for the more generic drop container, I agree, but it was something I was planning to do once we started implementing more container types.

crisbeto · 2018-12-05T06:59:43Z

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

+    @Inject(CDK_DRAG_CONFIG) private _config: DragRefConfig,
+    @Optional() private _dir: Directionality) {
+
+    const ref = this._dragRef = new DragRef(element, this._document, this._ngZone,


Agreed, my plan was to add a service for it in a follow-up. The point of this PR was to get the more difficult part of the refactor out of the way so that we can focus on the public API for it afterwards.

crisbeto · 2018-12-05T07:03:14Z

src/cdk/drag-drop/directives/drop-list.ts

+let _uniqueIdCounter = 0;
+
+// tslint:disable:class-name
+// TODO: exclude from public-api.ts


Isn't this the purpose of the @internal tag? I know that we removed them to avoid downstream issues, but the symbols should be preserved if we didn't enable stripInternal.

crisbeto · 2018-12-05T07:04:07Z

src/cdk/drag-drop/directives/drop-list.ts

+   * @param x Pointer position along the X axis.
+   * @param y Pointer position along the Y axis.
+   */
+  _isOverContainer(x: number, y: number): boolean {


It can be, I left it in to keep the CdkDropList compatible with the CdkDropListContainer interfaces.

crisbeto · 2018-12-05T19:23:48Z

All of the feedback has been addressed.

jelbourn

LGTM

ngbot · 2018-12-05T19:41:14Z

I see that you just added the pr: merge ready label, but the following checks are still failing:
conflicts with base branch "master"
status "ci/circleci: bazel_build_test" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

Moves the logic for `CdkDrag` and `CdkDropList` into separate, non-Angular-specific classes. This is a first step towards allowing consumers to attach drag&drop functionality to arbitrary DOM nodes.

This reverts commit 19f9bca.

…from ListKeyManager" (#14467) * Revert "build: update sauce connect (#14422)" This reverts commit bc563b7. * Revert "build: changelog script not filtering duplicate entries (#14421)" This reverts commit fe46290. * Revert "build: update to angular 7.1.2 (#14418)" This reverts commit e1487df. * Revert "refactor(drag-drop): move logic out of directives (#14350)" This reverts commit 19f9bca. * Revert "fix(checkbox): redirect focus to underlying input element (#13959)" This reverts commit e0eb3df. * Revert "fix(radio): host element unable to receive focus events (#13956)" This reverts commit 41eb27e. * Revert "fix(a11y): activeItem out of date if active index is removed from ListKeyManager (#14407)" This reverts commit 014dc79.

This reverts commit 19f9bca.

) * Revert "Revert "fix(a11y): activeItem out of date if active index is removed from ListKeyManager" (#14467)" This reverts commit 8f790f5. * Revert "build: update sauce connect (#14422)" This reverts commit bc563b7. * Revert "build: changelog script not filtering duplicate entries (#14421)" This reverts commit fe46290. * Revert "build: update to angular 7.1.2 (#14418)" This reverts commit e1487df. * Revert "refactor(drag-drop): move logic out of directives (#14350)" This reverts commit 19f9bca. * Revert "fix(checkbox): redirect focus to underlying input element (#13959)" This reverts commit e0eb3df. * Revert "fix(radio): host element unable to receive focus events (#13956)" This reverts commit 41eb27e.

…14469) * Revert "Revert "fix(radio): host element unable to receive focus events" (#14468)" This reverts commit 4aa47c7. * Revert "Revert "fix(a11y): activeItem out of date if active index is removed from ListKeyManager" (#14467)" This reverts commit 8f790f5. * Revert "build: update sauce connect (#14422)" This reverts commit bc563b7. * Revert "build: changelog script not filtering duplicate entries (#14421)" This reverts commit fe46290. * Revert "build: update to angular 7.1.2 (#14418)" This reverts commit e1487df. * Revert "refactor(drag-drop): move logic out of directives (#14350)" This reverts commit 19f9bca. * Revert "fix(checkbox): redirect focus to underlying input element (#13959)" This reverts commit e0eb3df.

…from ListKeyManager" (#14467) * Revert "build: update sauce connect (#14422)" This reverts commit bc563b7. * Revert "build: changelog script not filtering duplicate entries (#14421)" This reverts commit fe46290. * Revert "build: update to angular 7.1.2 (#14418)" This reverts commit e1487df. * Revert "refactor(drag-drop): move logic out of directives (#14350)" This reverts commit 19f9bca. * Revert "fix(checkbox): redirect focus to underlying input element (#13959)" This reverts commit e0eb3df. * Revert "fix(radio): host element unable to receive focus events (#13956)" This reverts commit 41eb27e. * Revert "fix(a11y): activeItem out of date if active index is removed from ListKeyManager (#14407)" This reverts commit 014dc79.

) * Revert "Revert "fix(a11y): activeItem out of date if active index is removed from ListKeyManager" (#14467)" This reverts commit 8f790f5. * Revert "build: update sauce connect (#14422)" This reverts commit bc563b7. * Revert "build: changelog script not filtering duplicate entries (#14421)" This reverts commit fe46290. * Revert "build: update to angular 7.1.2 (#14418)" This reverts commit e1487df. * Revert "refactor(drag-drop): move logic out of directives (#14350)" This reverts commit 19f9bca. * Revert "fix(checkbox): redirect focus to underlying input element (#13959)" This reverts commit e0eb3df. * Revert "fix(radio): host element unable to receive focus events (#13956)" This reverts commit 41eb27e.

…14469) * Revert "Revert "fix(radio): host element unable to receive focus events" (#14468)" This reverts commit 4aa47c7. * Revert "Revert "fix(a11y): activeItem out of date if active index is removed from ListKeyManager" (#14467)" This reverts commit 8f790f5. * Revert "build: update sauce connect (#14422)" This reverts commit bc563b7. * Revert "build: changelog script not filtering duplicate entries (#14421)" This reverts commit fe46290. * Revert "build: update to angular 7.1.2 (#14418)" This reverts commit e1487df. * Revert "refactor(drag-drop): move logic out of directives (#14350)" This reverts commit 19f9bca. * Revert "fix(checkbox): redirect focus to underlying input element (#13959)" This reverts commit e0eb3df.

Moves the logic for `CdkDrag` and `CdkDropList` into separate, non-Angular-specific classes. This is a first step towards allowing consumers to attach drag&drop functionality to arbitrary DOM nodes.

…from ListKeyManager" (angular#14467) * Revert "build: update sauce connect (angular#14422)" This reverts commit bc563b7. * Revert "build: changelog script not filtering duplicate entries (angular#14421)" This reverts commit fe46290. * Revert "build: update to angular 7.1.2 (angular#14418)" This reverts commit e1487df. * Revert "refactor(drag-drop): move logic out of directives (angular#14350)" This reverts commit 19f9bca. * Revert "fix(checkbox): redirect focus to underlying input element (angular#13959)" This reverts commit e0eb3df. * Revert "fix(radio): host element unable to receive focus events (angular#13956)" This reverts commit 41eb27e. * Revert "fix(a11y): activeItem out of date if active index is removed from ListKeyManager (angular#14407)" This reverts commit 014dc79.

…ular#14468) * Revert "Revert "fix(a11y): activeItem out of date if active index is removed from ListKeyManager" (angular#14467)" This reverts commit 8f790f5. * Revert "build: update sauce connect (angular#14422)" This reverts commit bc563b7. * Revert "build: changelog script not filtering duplicate entries (angular#14421)" This reverts commit fe46290. * Revert "build: update to angular 7.1.2 (angular#14418)" This reverts commit e1487df. * Revert "refactor(drag-drop): move logic out of directives (angular#14350)" This reverts commit 19f9bca. * Revert "fix(checkbox): redirect focus to underlying input element (angular#13959)" This reverts commit e0eb3df. * Revert "fix(radio): host element unable to receive focus events (angular#13956)" This reverts commit 41eb27e.

…ngular#14469) * Revert "Revert "fix(radio): host element unable to receive focus events" (angular#14468)" This reverts commit 4aa47c7. * Revert "Revert "fix(a11y): activeItem out of date if active index is removed from ListKeyManager" (angular#14467)" This reverts commit 8f790f5. * Revert "build: update sauce connect (angular#14422)" This reverts commit bc563b7. * Revert "build: changelog script not filtering duplicate entries (angular#14421)" This reverts commit fe46290. * Revert "build: update to angular 7.1.2 (angular#14418)" This reverts commit e1487df. * Revert "refactor(drag-drop): move logic out of directives (angular#14350)" This reverts commit 19f9bca. * Revert "fix(checkbox): redirect focus to underlying input element (angular#13959)" This reverts commit e0eb3df.

angular-automatic-lock-bot · 2019-09-10T06:01: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 added the target: patch This PR is targeted for the next patch release label Dec 2, 2018

crisbeto assigned jelbourn Dec 2, 2018

crisbeto requested review from devversion and jelbourn as code owners December 2, 2018 15:13

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

crisbeto force-pushed the drag-drop-refactor branch from 15c21f3 to f2b6d66 Compare December 2, 2018 15:24

jelbourn reviewed Dec 4, 2018

View reviewed changes

crisbeto commented Dec 5, 2018

View reviewed changes

crisbeto force-pushed the drag-drop-refactor branch from f2b6d66 to 219beb1 Compare December 5, 2018 19:22

jelbourn approved these changes Dec 5, 2018

View reviewed changes

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

crisbeto force-pushed the drag-drop-refactor branch 2 times, most recently from 15db9c6 to 42ca449 Compare December 5, 2018 20:33

mmalerba added the P2 The issue is important to a large percentage of users, with a workaround label Dec 5, 2018

refactor(drag-drop): move logic out of directives

58e8142

Moves the logic for `CdkDrag` and `CdkDropList` into separate, non-Angular-specific classes. This is a first step towards allowing consumers to attach drag&drop functionality to arbitrary DOM nodes.

crisbeto force-pushed the drag-drop-refactor branch from 42ca449 to 58e8142 Compare December 5, 2018 22:12

mmalerba merged commit 19f9bca into angular:master Dec 7, 2018

crisbeto mentioned this pull request Dec 9, 2018

refactor(drag-drop): expose more private apis as protected #14134

Closed

mmalerba added a commit that referenced this pull request Dec 11, 2018

Revert "refactor(drag-drop): move logic out of directives (#14350)"

56ee441

This reverts commit 19f9bca.

mmalerba added a commit that referenced this pull request Dec 11, 2018

Revert "refactor(drag-drop): move logic out of directives (#14350)"

8b20c09

This reverts commit 19f9bca.

mmalerba added a commit that referenced this pull request Dec 11, 2018

Revert "refactor(drag-drop): move logic out of directives (#14350)"

456cc76

This reverts commit 19f9bca.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(drag-drop): move logic out of directives #14350

refactor(drag-drop): move logic out of directives #14350

crisbeto commented Dec 2, 2018

ngbot bot commented Dec 3, 2018

jelbourn left a comment

jelbourn Dec 4, 2018

jelbourn Dec 4, 2018

jelbourn Dec 4, 2018

crisbeto Dec 5, 2018

jelbourn Dec 4, 2018

crisbeto Dec 5, 2018

jelbourn Dec 4, 2018

crisbeto Dec 5, 2018

jelbourn Dec 5, 2018

jelbourn Dec 4, 2018

jelbourn Dec 4, 2018

jelbourn Dec 4, 2018

jelbourn Dec 4, 2018

crisbeto Dec 5, 2018

jelbourn Dec 4, 2018

crisbeto left a comment

crisbeto Dec 5, 2018

crisbeto Dec 5, 2018

crisbeto Dec 5, 2018

crisbeto Dec 5, 2018

crisbeto commented Dec 5, 2018

jelbourn left a comment

ngbot bot commented Dec 5, 2018

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



		// tslint:disable:class-name
		// TODO: exclude from public-api.ts

refactor(drag-drop): move logic out of directives #14350

refactor(drag-drop): move logic out of directives #14350

Conversation

crisbeto commented Dec 2, 2018

ngbot bot commented Dec 3, 2018

jelbourn left a 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

Choose a reason for hiding this comment

crisbeto left a 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 Dec 5, 2018

jelbourn left a comment

Choose a reason for hiding this comment

ngbot bot commented Dec 5, 2018

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