From db360d2c7f721d310b18f5e3dd24a723a4fd6911 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Tue, 13 Nov 2018 13:12:26 +1100 Subject: [PATCH 1/5] fixing tests --- .../when-reordering.js | 3 ++ .../move-relative-to.js | 32 +++++++++++++++---- .../get-page-border-box-center.spec.js | 3 ++ .../move-relative-to.spec.js | 2 ++ 4 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/state/get-center-from-impact/get-page-border-box-center/when-reordering.js b/src/state/get-center-from-impact/get-page-border-box-center/when-reordering.js index 3ebb0ba529..432aad1fd4 100644 --- a/src/state/get-center-from-impact/get-page-border-box-center/when-reordering.js +++ b/src/state/get-center-from-impact/get-page-border-box-center/when-reordering.js @@ -67,6 +67,7 @@ export default ({ axis, moveRelativeTo: moveRelativeTo.page, isMoving: draggablePage, + isOverHome, }); } @@ -78,6 +79,7 @@ export default ({ axis, moveRelativeTo: displacedClosest, isMoving: draggablePage, + isOverHome, }); } @@ -86,5 +88,6 @@ export default ({ axis, moveRelativeTo: displacedClosest, isMoving: draggablePage, + isOverHome, }); }; diff --git a/src/state/get-center-from-impact/move-relative-to.js b/src/state/get-center-from-impact/move-relative-to.js index 35b32c71ad..dd59756dc8 100644 --- a/src/state/get-center-from-impact/move-relative-to.js +++ b/src/state/get-center-from-impact/move-relative-to.js @@ -7,6 +7,7 @@ type Args = {| axis: Axis, moveRelativeTo: BoxModel, isMoving: BoxModel, + isOverHome: boolean, |}; const distanceFromStartToCenter = (axis: Axis, box: BoxModel): number => @@ -21,24 +22,43 @@ const distanceFromEndToCenter = (axis: Axis, box: BoxModel): number => box.padding[axis.end] + box.contentBox[axis.size] / 2; -export const goAfter = ({ axis, moveRelativeTo, isMoving }: Args): Position => +const getCrossAxisCenter = ({ + axis, + moveRelativeTo, + isMoving, + isOverHome, +}: Args): number => { + const target: BoxModel = isOverHome ? isMoving : moveRelativeTo; + + return target.borderBox.center[axis.crossAxisLine]; +}; + +export const goAfter = ({ + axis, + moveRelativeTo, + isMoving, + isOverHome, +}: Args): Position => patch( axis.line, // start measuring from the bottom of the target moveRelativeTo.marginBox[axis.end] + distanceFromStartToCenter(axis, isMoving), - // align the moving item to the visual center of the target - moveRelativeTo.borderBox.center[axis.crossAxisLine], + getCrossAxisCenter({ axis, moveRelativeTo, isMoving, isOverHome }), ); -export const goBefore = ({ axis, moveRelativeTo, isMoving }: Args): Position => +export const goBefore = ({ + axis, + moveRelativeTo, + isMoving, + isOverHome, +}: Args): Position => patch( axis.line, // start measuring from the top of the target moveRelativeTo.marginBox[axis.start] - distanceFromEndToCenter(axis, isMoving), - // align the moving item to the visual center of the target - moveRelativeTo.borderBox.center[axis.crossAxisLine], + getCrossAxisCenter({ axis, moveRelativeTo, isMoving, isOverHome }), ); type GoIntoArgs = {| diff --git a/test/unit/state/get-center-from-impact/get-page-border-box-center.spec.js b/test/unit/state/get-center-from-impact/get-page-border-box-center.spec.js index 9f8bec47ed..f8c438d24d 100644 --- a/test/unit/state/get-center-from-impact/get-page-border-box-center.spec.js +++ b/test/unit/state/get-center-from-impact/get-page-border-box-center.spec.js @@ -104,6 +104,7 @@ const getDisplacement = (draggable: DraggableDimension): Displacement => ({ axis, moveRelativeTo: displacedInHome3, isMoving: preset.inHome1.page, + isOverHome: true, }); expect(result).toEqual(expectedCenter); }); @@ -152,6 +153,7 @@ const getDisplacement = (draggable: DraggableDimension): Displacement => ({ axis, moveRelativeTo: displacedInHome1, isMoving: preset.inHome3.page, + isOverHome: true, }); expect(result).toEqual(expectedCenter); }); @@ -192,6 +194,7 @@ const getDisplacement = (draggable: DraggableDimension): Displacement => ({ axis, moveRelativeTo: preset.inForeign4.page, isMoving: preset.inHome1.page, + isOverHome: false, }); expect(result).toEqual(expectedCenter); }); diff --git a/test/unit/state/get-center-from-impact/move-relative-to.spec.js b/test/unit/state/get-center-from-impact/move-relative-to.spec.js index 6b45ebc2e9..9166847ad1 100644 --- a/test/unit/state/get-center-from-impact/move-relative-to.spec.js +++ b/test/unit/state/get-center-from-impact/move-relative-to.spec.js @@ -53,6 +53,7 @@ const isMoving: BoxModel = createBox({ axis, moveRelativeTo, isMoving, + isOverHome: false, }); const expected: Position = patch( @@ -73,6 +74,7 @@ const isMoving: BoxModel = createBox({ axis, moveRelativeTo, isMoving, + isOverHome: false, }); const expected: Position = patch( From 29704a84928a7e38a791bfdb201e21ccfb2b55e3 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Tue, 13 Nov 2018 13:27:42 +1100 Subject: [PATCH 2/5] move relative to spec - with home list --- .../move-relative-to.spec.js | 159 ++++++++++++------ 1 file changed, 106 insertions(+), 53 deletions(-) diff --git a/test/unit/state/get-center-from-impact/move-relative-to.spec.js b/test/unit/state/get-center-from-impact/move-relative-to.spec.js index 9166847ad1..3ab986cf64 100644 --- a/test/unit/state/get-center-from-impact/move-relative-to.spec.js +++ b/test/unit/state/get-center-from-impact/move-relative-to.spec.js @@ -48,65 +48,118 @@ const isMoving: BoxModel = createBox({ }); [vertical, horizontal].forEach((axis: Axis) => { - it('should align before the target', () => { - const newCenter: Position = goBefore({ - axis, - moveRelativeTo, - isMoving, - isOverHome: false, - }); + describe(`on ${axis.direction} axis`, () => { + it('should move into the start of the context box of the target', () => { + const newCenter: Position = goIntoStart({ + axis, + moveInto: moveRelativeTo, + isMoving, + }); - const expected: Position = patch( - axis.line, - moveRelativeTo.marginBox[axis.start] - - (isMoving.margin[axis.end] + - isMoving.border[axis.end] + - isMoving.padding[axis.end] + - isMoving.contentBox[axis.size] / 2), - moveRelativeTo.borderBox.center[axis.crossAxisLine], - ); - - expect(newCenter).toEqual(expected); - }); + const expected: Position = patch( + axis.line, + moveRelativeTo.contentBox[axis.start] + + isMoving.margin[axis.start] + + isMoving.border[axis.start] + + isMoving.padding[axis.start] + + isMoving.contentBox[axis.size] / 2, + // move on the same cross axis as the list we are moving into + moveRelativeTo.contentBox.center[axis.crossAxisLine], + ); - it('should align after the target', () => { - const newCenter: Position = goAfter({ - axis, - moveRelativeTo, - isMoving, - isOverHome: false, + expect(newCenter).toEqual(expected); }); - const expected: Position = patch( - axis.line, - moveRelativeTo.marginBox[axis.end] + - isMoving.margin[axis.start] + - isMoving.border[axis.start] + - isMoving.padding[axis.start] + - isMoving.contentBox[axis.size] / 2, - moveRelativeTo.borderBox.center[axis.crossAxisLine], - ); - - expect(newCenter).toEqual(expected); - }); + describe('is over home list', () => { + it('should align before the target', () => { + const newCenter: Position = goBefore({ + axis, + moveRelativeTo, + isMoving, + isOverHome: true, + }); + + const expected: Position = patch( + axis.line, + moveRelativeTo.marginBox[axis.start] - + (isMoving.margin[axis.end] + + isMoving.border[axis.end] + + isMoving.padding[axis.end] + + isMoving.contentBox[axis.size] / 2), + // move on the same cross axis as where the item started + isMoving.borderBox.center[axis.crossAxisLine], + ); + + expect(newCenter).toEqual(expected); + }); + + it('should align after the target', () => { + const newCenter: Position = goAfter({ + axis, + moveRelativeTo, + isMoving, + isOverHome: true, + }); + + const expected: Position = patch( + axis.line, + moveRelativeTo.marginBox[axis.end] + + isMoving.margin[axis.start] + + isMoving.border[axis.start] + + isMoving.padding[axis.start] + + isMoving.contentBox[axis.size] / 2, + // move on the same cross axis as where the item started + isMoving.borderBox.center[axis.crossAxisLine], + ); - it('should move into the start of the context box of the target', () => { - const newCenter: Position = goIntoStart({ - axis, - moveInto: moveRelativeTo, - isMoving, + expect(newCenter).toEqual(expected); + }); }); - const expected: Position = patch( - axis.line, - moveRelativeTo.contentBox[axis.start] + - isMoving.margin[axis.start] + - isMoving.border[axis.start] + - isMoving.padding[axis.start] + - isMoving.contentBox[axis.size] / 2, - moveRelativeTo.contentBox.center[axis.crossAxisLine], - ); - - expect(newCenter).toEqual(expected); + describe('is over foreign list', () => { + it('should align before the target', () => { + const newCenter: Position = goBefore({ + axis, + moveRelativeTo, + isMoving, + isOverHome: false, + }); + + const expected: Position = patch( + axis.line, + moveRelativeTo.marginBox[axis.start] - + (isMoving.margin[axis.end] + + isMoving.border[axis.end] + + isMoving.padding[axis.end] + + isMoving.contentBox[axis.size] / 2), + // move on the cross axis of the target + moveRelativeTo.borderBox.center[axis.crossAxisLine], + ); + + expect(newCenter).toEqual(expected); + }); + + it('should align after the target', () => { + const newCenter: Position = goAfter({ + axis, + moveRelativeTo, + isMoving, + isOverHome: false, + }); + + const expected: Position = patch( + axis.line, + moveRelativeTo.marginBox[axis.end] + + isMoving.margin[axis.start] + + isMoving.border[axis.start] + + isMoving.padding[axis.start] + + isMoving.contentBox[axis.size] / 2, + // move on the cross axis of the target + moveRelativeTo.borderBox.center[axis.crossAxisLine], + ); + + expect(newCenter).toEqual(expected); + }); + }); }); }); From 961632815172d36e4293da702917f00daac2c44f Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Tue, 13 Nov 2018 14:04:26 +1100 Subject: [PATCH 3/5] adding spacing fix --- .../move-relative-to.spec.js | 47 +++++++++++-------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/test/unit/state/get-center-from-impact/move-relative-to.spec.js b/test/unit/state/get-center-from-impact/move-relative-to.spec.js index 3ab986cf64..11be4aafa4 100644 --- a/test/unit/state/get-center-from-impact/move-relative-to.spec.js +++ b/test/unit/state/get-center-from-impact/move-relative-to.spec.js @@ -47,6 +47,18 @@ const isMoving: BoxModel = createBox({ padding: getAssortedSpacing(), }); +const distanceFromStartToCenter = (axis: Axis, box: BoxModel): number => + box.margin[axis.start] + + box.border[axis.start] + + box.padding[axis.start] + + box.contentBox[axis.size] / 2; + +const distanceFromEndToCenter = (axis: Axis, box: BoxModel): number => + box.margin[axis.end] + + box.border[axis.end] + + box.padding[axis.end] + + box.contentBox[axis.size] / 2; + [vertical, horizontal].forEach((axis: Axis) => { describe(`on ${axis.direction} axis`, () => { it('should move into the start of the context box of the target', () => { @@ -58,11 +70,10 @@ const isMoving: BoxModel = createBox({ const expected: Position = patch( axis.line, + // start from the start of the context box of the target moveRelativeTo.contentBox[axis.start] + - isMoving.margin[axis.start] + - isMoving.border[axis.start] + - isMoving.padding[axis.start] + - isMoving.contentBox[axis.size] / 2, + // add the distance from the start to the center of the moving item + distanceFromStartToCenter(axis, isMoving), // move on the same cross axis as the list we are moving into moveRelativeTo.contentBox.center[axis.crossAxisLine], ); @@ -81,11 +92,10 @@ const isMoving: BoxModel = createBox({ const expected: Position = patch( axis.line, + // start at the start of the item we are moving relative to moveRelativeTo.marginBox[axis.start] - - (isMoving.margin[axis.end] + - isMoving.border[axis.end] + - isMoving.padding[axis.end] + - isMoving.contentBox[axis.size] / 2), + // add the space from the end of the dragging item to its center + distanceFromEndToCenter(axis, isMoving), // move on the same cross axis as where the item started isMoving.borderBox.center[axis.crossAxisLine], ); @@ -103,11 +113,10 @@ const isMoving: BoxModel = createBox({ const expected: Position = patch( axis.line, + // start at the end of the margin box moveRelativeTo.marginBox[axis.end] + - isMoving.margin[axis.start] + - isMoving.border[axis.start] + - isMoving.padding[axis.start] + - isMoving.contentBox[axis.size] / 2, + // add the distance to the start of the target center + distanceFromStartToCenter(axis, isMoving), // move on the same cross axis as where the item started isMoving.borderBox.center[axis.crossAxisLine], ); @@ -127,11 +136,10 @@ const isMoving: BoxModel = createBox({ const expected: Position = patch( axis.line, + // start at the start of the target moveRelativeTo.marginBox[axis.start] - - (isMoving.margin[axis.end] + - isMoving.border[axis.end] + - isMoving.padding[axis.end] + - isMoving.contentBox[axis.size] / 2), + // subtract the space from end of the moving item to its center + distanceFromEndToCenter(axis, isMoving), // move on the cross axis of the target moveRelativeTo.borderBox.center[axis.crossAxisLine], ); @@ -149,11 +157,10 @@ const isMoving: BoxModel = createBox({ const expected: Position = patch( axis.line, + // start at the end of the target moveRelativeTo.marginBox[axis.end] + - isMoving.margin[axis.start] + - isMoving.border[axis.start] + - isMoving.padding[axis.start] + - isMoving.contentBox[axis.size] / 2, + // add the space from the start of the moving item to its center + distanceFromStartToCenter(axis, isMoving), // move on the cross axis of the target moveRelativeTo.borderBox.center[axis.crossAxisLine], ); From 0d294d17608a168ad2d3ef4a27f80c5b0236a079 Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Tue, 13 Nov 2018 14:39:26 +1100 Subject: [PATCH 4/5] adding comment --- src/state/get-center-from-impact/move-relative-to.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/state/get-center-from-impact/move-relative-to.js b/src/state/get-center-from-impact/move-relative-to.js index dd59756dc8..006c75579a 100644 --- a/src/state/get-center-from-impact/move-relative-to.js +++ b/src/state/get-center-from-impact/move-relative-to.js @@ -28,6 +28,11 @@ const getCrossAxisCenter = ({ isMoving, isOverHome, }: Args): number => { + // home list: use the center from the moving item + // foreign list: use the center from the box we are moving relative to + + // Sometimes we can be in a home list that has items of different sizes. + // Eg: columns in a horizontal list const target: BoxModel = isOverHome ? isMoving : moveRelativeTo; return target.borderBox.center[axis.crossAxisLine]; From 6881526f1d672403dcfc80a5479d4b8c93c1486e Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Tue, 13 Nov 2018 14:43:04 +1100 Subject: [PATCH 5/5] more comments --- src/state/get-center-from-impact/move-relative-to.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/state/get-center-from-impact/move-relative-to.js b/src/state/get-center-from-impact/move-relative-to.js index 006c75579a..b65db2349c 100644 --- a/src/state/get-center-from-impact/move-relative-to.js +++ b/src/state/get-center-from-impact/move-relative-to.js @@ -28,11 +28,14 @@ const getCrossAxisCenter = ({ isMoving, isOverHome, }: Args): number => { + // Sometimes we can be in a home list that has items of different sizes. + // Eg: columns in a horizontal list + // home list: use the center from the moving item // foreign list: use the center from the box we are moving relative to - // Sometimes we can be in a home list that has items of different sizes. - // Eg: columns in a horizontal list + // TODO: what if a foreign list has items of different sizes? + const target: BoxModel = isOverHome ? isMoving : moveRelativeTo; return target.borderBox.center[axis.crossAxisLine];