feat(drag-drop): add service for attaching drag&drop to arbitrary DOM nodes #14437
Conversation
googlebot
added
the
cla: yes
| createTouchEvent, | ||
| } from '@angular/cdk/testing'; | ||
| import {Directionality} from '@angular/cdk/bidi'; | ||
| import {of as observbleOf} from 'rxjs'; |
There was a problem hiding this comment.
Ops, thanks for catching it.
crisbeto
force-pushed
the
drag-drop-service
branch
from
6a4e57a to
6873cda
Compare
crisbeto
force-pushed
the
drag-drop-service
branch
from
6873cda to
60ceb18
Compare
|
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
crisbeto
force-pushed
the
drag-drop-service
branch
from
60ceb18 to
1156277
Compare
jelbourn
left a comment
jelbourn
left a comment
There was a problem hiding this comment.
Do you plan to add documentation for the service in a follow-up PR?
src/cdk/drag-drop/directives/drag.ts
Outdated
| .withPlaceholderTemplate(this._placeholderTemplate ? { | ||
| templateRef: this._placeholderTemplate.templateRef, | ||
| data: this._placeholderTemplate.data, | ||
| viewContainer: this._viewContainerRef |
There was a problem hiding this comment.
I would move the placeholder and preview ternary expressions out into separate variables so that the .with calls are a bit more concise here
src/cdk/drag-drop/drag-drop.ts
Outdated
| import {DragDropRegistry} from './drag-drop-registry'; | ||
|
|
||
| /** | ||
| * Service that allows for drag&drop functionality to be attached to DOM elements. |
src/cdk/drag-drop/drag-drop.ts
Outdated
| config: DragRefConfig = { | ||
| dragStartThreshold: 5, | ||
| pointerDirectionChangeThreshold: 5 | ||
| }): DragRef<T> { |
There was a problem hiding this comment.
Move this to a DEFAULT_CONFIG const?
| * Service that allows for drag&drop functionality to be attached to DOM elements. | ||
| */ | ||
| @Injectable({providedIn: 'root'}) | ||
| export class DragDrop { |
There was a problem hiding this comment.
Add unit tests for consuming this service?
crisbeto
force-pushed
the
drag-drop-service
branch
from
1156277 to
092b4c0
Compare
|
@jelbourn I've addressed the feedback. Regarding docs, I was planning on adding some live examples in a follow-up. As for tests, I'm not sure whether it makes sense to test this service, because all it does is create the refs which is covered by the TS type checking already. |
|
It would be good to have at least one test that verifies the services can be injected and invoked correctly from a code coverage standpoint. |
crisbeto
force-pushed
the
drag-drop-service
branch
3 times, most recently
from
96238ef to
6a3eb5f
Compare
… nodes * Adds the `DragDrop` service that simplifies the construction logic for `DragRef` and `DropListRef` and allows consumers to attach drag&drop functionality to arbitrary DOM nodes, rather than having to go through the directives. * Reworks `DragRef` and `DragDropRef` to make them easier to construct. * Normalizes some of the instances where some parameters only accept an `ElementRef`, whereas others accept `ElementRef | HTMLElement`.
crisbeto
force-pushed
the
drag-drop-service
branch
from
6a3eb5f to
1efd7a4
Compare
|
@jelbourn this PR should be good to go now. |
jelbourn
added
pr: lgtm
action: merge
|
I see that you just added the 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. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
5 participants
DragDropservice that simplifies the construction logic forDragRefandDropListRefand allows consumers to attach drag&drop functionality to arbitrary DOM nodes, rather than having to go through the directives.DragRefandDragDropRefto make them easier to construct.ElementRef, whereas others acceptElementRef | HTMLElement.