fix(drag-drop): incorrectly laying out items with different height or margins #13849
Conversation
crisbeto
added
P2
googlebot
added
the
cla: yes
src/cdk/drag-drop/drop-list.ts
Outdated
| @@ -37,6 +37,34 @@ let _uniqueIdCounter = 0; | |||
| */ | |||
| const DROP_PROXIMITY_THRESHOLD = 0.05; | |||
|
|
|||
| /** Object used to cache the position of a drag list, its items and siblings. */ | |||
There was a problem hiding this comment.
@docs-private for this and the other related interfaces?
nit: add comma after "items"
There was a problem hiding this comment.
AFAIK they don't need to have docs-private, because they're not exported.
src/cdk/drag-drop/drop-list.ts
Outdated
| @@ -449,6 +471,56 @@ export class CdkDropList<T = any> implements OnInit, OnDestroy { | |||
| return pointerY > top - yThreshold && pointerY < bottom + yThreshold && | |||
| pointerX > left - xThreshold && pointerX < right + xThreshold; | |||
| } | |||
|
|
|||
| /** | |||
| * Gets the offset by which the item that is being dragged should be moved. | |||
There was a problem hiding this comment.
Function doesn't make clear whether this is an index offset or a pixel offset. Maybe getItemOffsetPx?
src/cdk/drag-drop/drop-list.ts
Outdated
| * @param newPosition Position of the item where the current item should be moved. | ||
| * @param delta Direction in which the user is moving. | ||
| */ | ||
| private _getItemOffset(currentPosition: ClientRect, newPosition: ClientRect, delta: number) { |
| newPosition.top - currentPosition.top; | ||
|
|
||
| // Account for differences in the item width/height. | ||
| if (delta === -1) { |
There was a problem hiding this comment.
I don't quite follow this. If you're always dealing with the top/left position, how does the difference in size come into play?
There was a problem hiding this comment.
It comes into play when you're swapping a tall item with a shorter one. If we only swapped their top, the layout would be messed up, because there'll be extra space under the short item and and the taller one will overlap it.
| const start = isHorizontal ? 'left' : 'top'; | ||
| const end = isHorizontal ? 'right' : 'bottom'; | ||
|
|
||
| if (delta === -1) { |
There was a problem hiding this comment.
I also don't quite follow the calculation that's going on here- could you clarify it a bit further?
There was a problem hiding this comment.
I've expanded the comment.
… margins Currently the `CdkDropList` makes some assumptions about the items not having a margin between each other. It also won't lay out and animate items with varying heights, in some cases. The following changes rework the directive to allow it to handle both varying heights and different margins. Fixes angular#13483.
crisbeto
force-pushed
the
13483/drag-drop-uneven-spacing
branch
from
6867190 to
d5d5030
Compare
|
I've addressed the feedback @jelbourn. |
jelbourn
added
pr: lgtm
action: merge
… margins (angular#13849) Currently the `CdkDropList` makes some assumptions about the items not having a margin between each other. It also won't lay out and animate items with varying heights, in some cases. The following changes rework the directive to allow it to handle both varying heights and different margins. Fixes angular#13483.
… margins (#13849) Currently the `CdkDropList` makes some assumptions about the items not having a margin between each other. It also won't lay out and animate items with varying heights, in some cases. The following changes rework the directive to allow it to handle both varying heights and different margins. Fixes #13483.
|
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
CdkDropListmakes some assumptions about the items not having a margin between each other. It also won't lay out and animate items with varying heights, in some cases. The following changes rework the directive to allow it to handle both varying heights and different margins.Fixes #13483.
Note: marking as a P2, because this a long-standing issue on a somewhat common use case.