fix(drag-drop): sorting too often if pointer is inside element after position swap #12837
Conversation
crisbeto
added
pr: merge safe
target: major
googlebot
added
the
cla: yes
src/cdk/drag-drop/drag.ts
Outdated
| * Amount the pixels the user should drag before we | ||
| * consider them to have changed the drag direction. | ||
| */ | ||
| const DELTA_CHANGE_THRESHOLD = 5; |
There was a problem hiding this comment.
How about POINTER_DIRECTION_CHANGE_THRESHOLD?
src/cdk/drag-drop/drag.ts
Outdated
| @@ -114,6 +120,12 @@ export class CdkDrag<T = any> implements OnDestroy { | |||
| */ | |||
| private _moveEventSubscriptions = 0; | |||
|
|
|||
| /** Keeps track of the direction in which the user is dragging along each axis. */ | |||
| private _delta: { | |||
There was a problem hiding this comment.
It's not immediately clear to me what _delta.x / _delta.y represents
There was a problem hiding this comment.
I'm not sure what else to call it. Something like pointerDirection isn't much better.
There was a problem hiding this comment.
Is it just the x/y delta from the last move event?
There was a problem hiding this comment.
Yeah, it's either 1 or -1, depending on whether the user is going up/down or left/right, similar to the deltaX and deltaY on the WheelEvent.
There was a problem hiding this comment.
How about splitting it up into two properties?
private _pointerDirectionDelta: {x: -1 | 1, y: -1 | 1};
private _pointerPositionAtLastDirectionChange: Point;
src/cdk/drag-drop/drag.ts
Outdated
| @@ -590,6 +605,30 @@ export class CdkDrag<T = any> implements OnDestroy { | |||
|
|
|||
| this._placeholder = this._placeholderRef = null!; | |||
| } | |||
|
|
|||
| /** Updates the current drag delta, based on the user's current pointer position on the page. */ | |||
| private _updateDelta(pointerPositionOnPage: Point) { | |||
There was a problem hiding this comment.
_updatePointerDirectionDelta?
pointerPositionOnPage -> pointerPosition? (isn't it always on the page?)
I'm also wondering if would make sense to refactor most of the pointer-related code into a separate class like PointerState; this file is getting pretty hefty, and it would be nice to separate the logic of the drag/drop behavior from the Angular-specific stuff.
There was a problem hiding this comment.
WRT the parameter naming, I wanted to make the distinction that it's relative to the page, rather than within the element, which we also keep track of inside the drag.ts.
Regarding refactoring, I agree that we should do it before we end up in a situation like mat-select. I've added it to my list.
crisbeto
force-pushed
the
drag-drop-uneven-height-sorting
branch
from
ff5cb6f to
6d77a47
Compare
|
Feedback has been addressed. |
…position swap Currently we check whether the user's pointer overlaps an item for each pixel that they've moved. This works fine when the items have a similar height, however it breaks down if there's a position swap between a very tall item and a smaller one. These changes rework the logic to take into account the direction that the user is moving in, before doing a position swap. Fixes angular#12694.
crisbeto
force-pushed
the
drag-drop-uneven-height-sorting
branch
from
6d77a47 to
dfead91
Compare
jelbourn
added
pr: lgtm
action: merge
|
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. |
Currently we check whether the user's pointer overlaps an item for each pixel that they've moved. This works fine when the items have a similar height, however it breaks down if there's a position swap between a very tall item and a smaller one. These changes rework the logic to take into account the direction that the user is moving in, before doing a position swap.
Fixes #12694.
For reference, here's what the current behavior looks like:
