fix(drag-drop): don't move item in list if pointer is too far away #12827
Conversation
crisbeto
added
pr: merge safe
target: major
googlebot
added
the
cla: yes
src/cdk/drag-drop/drop.ts
Outdated
| * Buffer, in percent of the width/height, to add around the drop container when | ||
| * determining whether an item should affect the list order. | ||
| */ | ||
| const DROP_BUFFER_PERCENTAGE = 5; |
There was a problem hiding this comment.
I would call this DROP_PROXIMITY_THRESHOLD and set it to 0.05. It's generally better to write constants like this as a number between 0 and 1 rather than as a whole number percent because it's more explicit about what it represents.
I suspect that this will also need a min and max value for cases when the drop zone is very small or very large, but I'd have to try it out in different situations to tell.
There was a problem hiding this comment.
I think that eventually we should allow people to customize the value, rather than picking a min and max since this is very much down to personal preference. The 5% value should do for now, because it's small enough that even if a drop container takes up the entire viewport on a 1080p screen, it would still only be 96px.
src/cdk/drag-drop/drop.ts
Outdated
| * @param xOffset Coordinates along the X axis. | ||
| * @param yOffset Coordinates along the Y axis. | ||
| */ | ||
| private _isInRange(xOffset: number, yOffset: number): boolean { |
There was a problem hiding this comment.
How about _isPointerNearDropContainer(pointerX, pointerY)?
src/cdk/drag-drop/drop.ts
Outdated
| const {top, right, bottom, left, width, height} = this._positionCache.self; | ||
| const multiplier = DROP_BUFFER_PERCENTAGE / 100; | ||
| const xBuffer = width * multiplier; | ||
| const yBuffer = height * multiplier; |
There was a problem hiding this comment.
I would use threshold here instead of buffer
crisbeto
force-pushed
the
drag-drop-radius
branch
from
edfdb19 to
423cecb
Compare
|
Feedback has been addressed. |
src/cdk/drag-drop/drop.ts
Outdated
| * @param xOffset Coordinates along the X axis. | ||
| * @param yOffset Coordinates along the Y axis. | ||
| */ | ||
| private _isPointerNearDropContainer(xOffset: number, yOffset: number): boolean { |
There was a problem hiding this comment.
I snuck in a suggestion w/ the function name comment on the parameter names too: pointerX and pointerY
There was a problem hiding this comment.
I went with xOffset and yOffset, because it's what we call them in all the other functions.
There was a problem hiding this comment.
I think it would be good to rename them in a follow-up PR, then, if they all do represent the pointer position.
src/cdk/drag-drop/drop.ts
Outdated
| @@ -30,6 +30,12 @@ import {moveItemInArray} from './drag-utils'; | |||
| /** Counter used to generate unique ids for drop zones. */ | |||
| let _uniqueIdCounter = 0; | |||
|
|
|||
| /** | |||
| * Buffer, in percent of the width/height, to add around the drop container when | |||
There was a problem hiding this comment.
Need to update the description for the new name, something like
Proximity, as a ratio to width/height, at which a dragged item will affect the drop container.
Currently the drop container will move an item in a list, no matter how far away the pointer is, as long as it overlaps another item along the proper axis. This can be annoying to users, because it can lead to unwanted re-arrangements of the list. These changes rework the logic so that the list is only re-arranged if the pointer is inside the drop container or within a buffer around it.
crisbeto
force-pushed
the
drag-drop-radius
branch
from
423cecb to
bbf51b1
Compare
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. |
Currently the drop container will move an item in a list, no matter how far away the pointer is, as long as it overlaps another item along the proper axis. This can be annoying to users, because it can lead to unwanted re-arrangements of the list. These changes rework the logic so that the list is only re-arranged if the pointer is inside the drop container or within a buffer around it.
For reference, here's the current behavior:
