From 35d03301613c5bf69254583e22acd191fa2d17de Mon Sep 17 00:00:00 2001 From: danieldelcore Date: Wed, 4 Dec 2019 16:41:58 +1100 Subject: [PATCH 1/4] updated testcase for distance calculation --- test/unit/state/get-droppable-over/preferencing.spec.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/unit/state/get-droppable-over/preferencing.spec.js b/test/unit/state/get-droppable-over/preferencing.spec.js index e77e4f3f47..e5bb980a8f 100644 --- a/test/unit/state/get-droppable-over/preferencing.spec.js +++ b/test/unit/state/get-droppable-over/preferencing.spec.js @@ -52,7 +52,8 @@ const droppableSecondary: DroppableDimension = getDroppableDimension({ top: 1000, left: 1200, right: 1300, - bottom: 1100, + // This is really tall to test the distance calculation against varied lists + bottom: 8000, }, }); @@ -64,8 +65,8 @@ const droppableTertiary: DroppableDimension = getDroppableDimension({ }, borderBox: { top: 1000, - left: 1100, - right: 1200, + left: 1400, + right: 1500, bottom: 1100, }, }); From f40e208219f31e6d554284e3fad4ed908c5ae9b1 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 4 Dec 2019 16:43:29 +1100 Subject: [PATCH 2/4] new mechanism for measuring distance --- src/state/get-droppable-over.js | 49 ++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/src/state/get-droppable-over.js b/src/state/get-droppable-over.js index 0c570910b3..9bacd2dde8 100644 --- a/src/state/get-droppable-over.js +++ b/src/state/get-droppable-over.js @@ -9,7 +9,7 @@ import type { } from '../types'; import { toDroppableList } from './dimension-structures'; import isPositionInFrame from './visibility/is-position-in-frame'; -import { distance } from './position'; +import { distance, patch } from './position'; import isWithin from './is-within'; // https://stackoverflow.com/questions/306316/determine-if-two-rectangles-overlap-each-other @@ -33,17 +33,37 @@ type WithDistance = {| distance: number, id: DroppableId, |}; -function getFurthestAway( - home: DroppableDimension, - droppables: DroppableDimension[], -): ?DroppableId { - const center: Position = home.page.borderBox.center; - - const sorted: WithDistance[] = droppables - .map((item: DroppableDimension): WithDistance => { + +type GetFurthestArgs = {| + pageBorderBox: Rect, + draggable: DraggableDimension, + candidates: DroppableDimension[], +|}; + +function getFurthestAway({ + pageBorderBox, + draggable, + candidates, +}: GetFurthestArgs): ?DroppableId { + // We are measuring the distance from where the draggable started + // to where it is *hitting* the candidate + // The hit point might technically not be in the bounds of the candidate + // We are not comparing the centers + const startCenter: Position = draggable.page.borderBox.center; + const sorted: WithDistance[] = candidates + .map((candidate: DroppableDimension): WithDistance => { + const axis: Axis = candidate.axis; + const target: Position = patch( + candidate.axis.line, + // use the current center of the dragging item on the main axis + pageBorderBox.center[axis.line], + // use the center of the list on the cross axis + candidate.page.borderBox.center[axis.crossAxisLine], + ); + return { - id: item.descriptor.id, - distance: distance(center, item.page.borderBox.center), + id: candidate.descriptor.id, + distance: distance(startCenter, target), }; }) // largest value will be first @@ -127,6 +147,9 @@ export default function getDroppableOver({ // Multiple options returned // Should only occur with really large items // Going to use fallback: distance from home - const home: DroppableDimension = droppables[draggable.descriptor.droppableId]; - return getFurthestAway(home, candidates); + return getFurthestAway({ + pageBorderBox, + draggable, + candidates, + }); } From f34d0d36ac523966ff6ca92c945e7622cf2802cb Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Wed, 4 Dec 2019 16:49:41 +1100 Subject: [PATCH 3/4] adding some comments --- src/state/get-droppable-over.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/state/get-droppable-over.js b/src/state/get-droppable-over.js index 9bacd2dde8..8c7b9c9d43 100644 --- a/src/state/get-droppable-over.js +++ b/src/state/get-droppable-over.js @@ -45,10 +45,13 @@ function getFurthestAway({ draggable, candidates, }: GetFurthestArgs): ?DroppableId { + // We are not comparing the center of the home list with the target list as it would + // give preference to giant lists + // We are measuring the distance from where the draggable started // to where it is *hitting* the candidate - // The hit point might technically not be in the bounds of the candidate - // We are not comparing the centers + // Note: The hit point might technically not be in the bounds of the candidate + const startCenter: Position = draggable.page.borderBox.center; const sorted: WithDistance[] = candidates .map((candidate: DroppableDimension): WithDistance => { From 73aecc85a94bc79cc9bf300abd55862033b3d3dc Mon Sep 17 00:00:00 2001 From: danieldelcore Date: Wed, 4 Dec 2019 17:00:21 +1100 Subject: [PATCH 4/4] rename droppables --- .../get-droppable-over/preferencing.spec.js | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/test/unit/state/get-droppable-over/preferencing.spec.js b/test/unit/state/get-droppable-over/preferencing.spec.js index e5bb980a8f..755c6822e4 100644 --- a/test/unit/state/get-droppable-over/preferencing.spec.js +++ b/test/unit/state/get-droppable-over/preferencing.spec.js @@ -14,7 +14,7 @@ import getDroppableOver from '../../../../src/state/get-droppable-over'; import { toDroppableMap } from '../../../../src/state/dimension-structures'; import { afterCrossAxisPoint } from '../../../util/after-point'; -const droppableLarge: DroppableDimension = getDroppableDimension({ +const droppableOrigin: DroppableDimension = getDroppableDimension({ descriptor: { id: 'large', type: 'standard', @@ -28,7 +28,7 @@ const droppableLarge: DroppableDimension = getDroppableDimension({ }, }); -const droppableSmall: DroppableDimension = getDroppableDimension({ +const droppableFirst: DroppableDimension = getDroppableDimension({ descriptor: { id: 'small', type: 'standard', @@ -42,7 +42,7 @@ const droppableSmall: DroppableDimension = getDroppableDimension({ }, }); -const droppableSecondary: DroppableDimension = getDroppableDimension({ +const droppableSecond: DroppableDimension = getDroppableDimension({ descriptor: { id: 'secondary', type: 'standard', @@ -57,7 +57,7 @@ const droppableSecondary: DroppableDimension = getDroppableDimension({ }, }); -const droppableTertiary: DroppableDimension = getDroppableDimension({ +const droppableThird: DroppableDimension = getDroppableDimension({ descriptor: { id: 'tertiary', type: 'standard', @@ -75,10 +75,10 @@ const draggable: DraggableDimension = getDraggableDimension({ descriptor: { id: 'my draggable', index: 0, - type: droppableLarge.descriptor.type, - droppableId: droppableLarge.descriptor.id, + type: droppableOrigin.descriptor.type, + droppableId: droppableOrigin.descriptor.id, }, - borderBox: droppableLarge.client.borderBox, + borderBox: droppableOrigin.client.borderBox, }); /** @@ -87,28 +87,28 @@ const draggable: DraggableDimension = getDraggableDimension({ */ it('should prefer the furthest away droppable when multiple lists are hit', () => { const offset = getOffsetForCrossAxisEndEdge({ - crossAxisEndEdgeOn: droppableTertiary.page.borderBox.center, + crossAxisEndEdgeOn: droppableThird.page.borderBox.center, dragging: draggable.page.borderBox, - axis: droppableTertiary.axis, + axis: droppableThird.axis, }); const pageBorderBox: Rect = offsetRectByPosition( draggable.page.borderBox, - afterCrossAxisPoint(droppableTertiary.axis, offset), + afterCrossAxisPoint(droppableThird.axis, offset), ); const result = getDroppableOver({ pageBorderBox, draggable, droppables: toDroppableMap([ - droppableLarge, - droppableSmall, - droppableSecondary, - droppableTertiary, + droppableOrigin, + droppableFirst, + droppableSecond, + droppableThird, ]), }); - expect(result).toEqual(droppableTertiary.descriptor.id); + expect(result).toEqual(droppableThird.descriptor.id); }); /** @@ -117,26 +117,26 @@ it('should prefer the furthest away droppable when multiple lists are hit', () = */ it('should prefer the second furthest away droppable when multiple lists are hit', () => { const offset = getOffsetForCrossAxisEndEdge({ - crossAxisEndEdgeOn: droppableSecondary.page.borderBox.center, + crossAxisEndEdgeOn: droppableSecond.page.borderBox.center, dragging: draggable.page.borderBox, - axis: droppableSecondary.axis, + axis: droppableSecond.axis, }); const pageBorderBox: Rect = offsetRectByPosition( draggable.page.borderBox, - afterCrossAxisPoint(droppableSecondary.axis, offset), + afterCrossAxisPoint(droppableSecond.axis, offset), ); const result = getDroppableOver({ pageBorderBox, draggable, droppables: toDroppableMap([ - droppableLarge, - droppableSmall, - droppableSecondary, - droppableTertiary, + droppableOrigin, + droppableFirst, + droppableSecond, + droppableThird, ]), }); - expect(result).toEqual(droppableSecondary.descriptor.id); + expect(result).toEqual(droppableSecond.descriptor.id); });