From d3981ea49c2e410bfd3f7a7b33d62783ed6beb47 Mon Sep 17 00:00:00 2001 From: Prev Wong Date: Sun, 21 Jun 2020 23:25:20 +0800 Subject: [PATCH 1/2] fix: isDroppable rules --- packages/core/src/editor/NodeHelpers.ts | 35 +++++++++++-------- .../core/src/editor/tests/NodeHelpers.test.ts | 33 +++++++++++------ packages/core/src/tests/fixtures.ts | 34 ++++++++++++++---- packages/core/src/utils/createTestNode.ts | 14 ++++++-- 4 files changed, 82 insertions(+), 34 deletions(-) diff --git a/packages/core/src/editor/NodeHelpers.ts b/packages/core/src/editor/NodeHelpers.ts index 93419c922..7d1b4b510 100644 --- a/packages/core/src/editor/NodeHelpers.ts +++ b/packages/core/src/editor/NodeHelpers.ts @@ -168,6 +168,14 @@ export function NodeHelpers(state: EditorState, id: NodeId) { return true; } + const targetDeepNodes = nodeHelpers(targetNode.id).descendants(true); + + invariant( + !targetDeepNodes.includes(newParentNode.id) && + newParentNode.id !== targetNode.id, + ERROR_MOVE_TO_DESCENDANT + ); + const currentParentNode = targetNode.data.parent && state.nodes[targetNode.data.parent]; @@ -179,21 +187,18 @@ export function NodeHelpers(state: EditorState, id: NodeId) { ERROR_DUPLICATE_NODEID ); - const targetDeepNodes = nodeHelpers(targetNode.id).descendants(); - - invariant( - !targetDeepNodes.includes(newParentNode.id) && - newParentNode.id !== targetNode.id, - ERROR_MOVE_TO_DESCENDANT - ); - invariant( - currentParentNode.rules.canMoveOut( - targetNode, - currentParentNode, - nodeHelpers - ), - ERROR_MOVE_OUTGOING_PARENT - ); + // If the Node we're checking for is not the same as the currentParentNode + // Check if the currentParentNode allows the targetNode to be dragged out + if (node !== currentParentNode) { + invariant( + currentParentNode.rules.canMoveOut( + targetNode, + currentParentNode, + nodeHelpers + ), + ERROR_MOVE_OUTGOING_PARENT + ); + } return true; } catch (err) { diff --git a/packages/core/src/editor/tests/NodeHelpers.test.ts b/packages/core/src/editor/tests/NodeHelpers.test.ts index 036a9ad6e..988763f58 100644 --- a/packages/core/src/editor/tests/NodeHelpers.test.ts +++ b/packages/core/src/editor/tests/NodeHelpers.test.ts @@ -72,31 +72,37 @@ describe('NodeHelpers', () => { describe('descendants', () => { it('should return immediate child and linked node ids', () => { - expect(helper('canvas-node-reject-dnd').descendants()).toStrictEqual( - helper('canvas-node-reject-dnd').get().data.nodes + expect( + helper('canvas-node-reject-outgoing-dnd').descendants() + ).toStrictEqual( + helper('canvas-node-reject-outgoing-dnd').get().data.nodes ); }); describe('when "includeOnly" is unset', () => { it('should return all child and linked nodes', () => { expect( - helper('canvas-node-reject-dnd').descendants(true) + helper('canvas-node-reject-outgoing-dnd').descendants(true) ).toStrictEqual([ - ...documentWithVariousNodes.nodes['canvas-node-reject-dnd'].data - .nodes, + ...documentWithVariousNodes.nodes['canvas-node-reject-outgoing-dnd'] + .data.nodes, ...Object.values( documentWithVariousNodes.nodes['parent-of-linked-node'].data .linkedNodes || {} ), + ...documentWithVariousNodes.nodes['linked-node'].data.nodes, ]); }); }); describe('when "includeOnly" is set to childNodes', () => { it('should return all child nodes only', () => { expect( - helper('canvas-node-reject-dnd').descendants(true, 'childNodes') + helper('canvas-node-reject-outgoing-dnd').descendants( + true, + 'childNodes' + ) ).toStrictEqual([ - ...documentWithVariousNodes.nodes['canvas-node-reject-dnd'].data - .nodes, + ...documentWithVariousNodes.nodes['canvas-node-reject-outgoing-dnd'] + .data.nodes, ]); }); }); @@ -154,17 +160,24 @@ describe('NodeHelpers', () => { }); it("should return false if node's rule rejects incoming target", () => { expect( - helper('canvas-node-reject-dnd').isDroppable(secondaryButton.id) + helper('canvas-node-reject-incoming-dnd').isDroppable( + secondaryButton.id + ) ).toEqual(false); }); it("should return false if node's rule rejects outgoing target", () => { + expect( + helper( + 'canvas-node-reject-outgoing-dnd' + ).isDroppable('fixed-child-node', (err) => console.log(err)) + ).toEqual(true); expect(helper('canvas-node').isDroppable('fixed-child-node')).toEqual( false ); }); it('should return false if target is a descendant', () => { expect( - helper('parent-of-linked-node').isDroppable('canvas-node-reject-dnd') + helper('linked-node-child-canvas').isDroppable('parent-of-linked-node') ).toEqual(false); }); }); diff --git a/packages/core/src/tests/fixtures.ts b/packages/core/src/tests/fixtures.ts index 4f2223938..f442c7d74 100644 --- a/packages/core/src/tests/fixtures.ts +++ b/packages/core/src/tests/fixtures.ts @@ -121,13 +121,18 @@ export const documentWithCardState = { }, }; +// TODO: Find a better way to create test child nodes export const documentWithVariousNodes = { ...documentWithCardState, nodes: { ...documentWithCardState.nodes, 'canvas-node': createTestNode('canvas-node', { isCanvas: true, - nodes: ['node-reject-dnd', 'canvas-node-reject-dnd'], + nodes: [ + 'node-reject-dnd', + 'canvas-node-incoming-dnd', + 'canvas-node-reject-outgoing-dnd', + ], }), 'node-reject-dnd': createTestNode( 'node-reject-dnd', @@ -141,16 +146,28 @@ export const documentWithVariousNodes = { }, } ), - 'canvas-node-reject-dnd': createTestNode( - 'canvas-node-reject-dnd', + 'canvas-node-reject-incoming-dnd': createTestNode( + 'canvas-node-reject-incoming-dnd', { - nodes: ['fixed-child-node', 'parent-of-linked-node'], + nodes: [], parent: 'canvas-node', isCanvas: true, }, { rules: { canMoveIn: () => false, + }, + } + ), + 'canvas-node-reject-outgoing-dnd': createTestNode( + 'canvas-node-reject-outgoing-dnd', + { + nodes: ['fixed-child-node', 'parent-of-linked-node'], + parent: 'canvas-node', + isCanvas: true, + }, + { + rules: { canMoveOut: () => false, }, } @@ -159,11 +176,11 @@ export const documentWithVariousNodes = { parent: 'node-reject-dnd', }), 'fixed-child-node': createTestNode('fixed-child-node', { - parent: 'canvas-node-reject-dnd', + parent: 'canvas-node-reject-outgoing-dnd', }), 'parent-of-linked-node': createTestNode('parent-of-linked-node', { isCanvas: true, - parent: 'canvas-node-reject-dnd', + parent: 'canvas-node-reject-outgoing-dnd', linkedNodes: { test: 'linked-node', }, @@ -171,6 +188,11 @@ export const documentWithVariousNodes = { 'linked-node': createTestNode('linked-node', { isCanvas: true, parent: 'parent-of-linked-node', + nodes: ['linked-node-child-canvas'], + }), + 'linked-node-child-canvas': createTestNode('linked-node-child-canvas', { + isCanvas: true, + parent: 'linked-node', }), }, }; diff --git a/packages/core/src/utils/createTestNode.ts b/packages/core/src/utils/createTestNode.ts index 9c6370f31..3d1de782b 100644 --- a/packages/core/src/utils/createTestNode.ts +++ b/packages/core/src/utils/createTestNode.ts @@ -1,5 +1,8 @@ -export const createTestNode = (id, data, config = {}) => { +import { Node } from '../interfaces'; + +export const createTestNode = (id, data, config: Partial = {}) => { return { + ...config, id, data: { props: {}, @@ -9,12 +12,17 @@ export const createTestNode = (id, data, config = {}) => { ...data, }, related: {}, - events: { selected: false, dragged: false, hovered: false }, + events: { + selected: false, + dragged: false, + hovered: false, + ...config.events, + }, rules: { canMoveIn: () => true, canMoveOut: () => true, canDrag: () => true, + ...config.rules, }, - ...config, }; }; From 725733711e8b1bfa6650efdea227190e2abba116 Mon Sep 17 00:00:00 2001 From: Prev Wong Date: Mon, 22 Jun 2020 11:04:06 +0800 Subject: [PATCH 2/2] nit --- packages/core/src/editor/tests/NodeHelpers.test.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/core/src/editor/tests/NodeHelpers.test.ts b/packages/core/src/editor/tests/NodeHelpers.test.ts index 988763f58..224de40bf 100644 --- a/packages/core/src/editor/tests/NodeHelpers.test.ts +++ b/packages/core/src/editor/tests/NodeHelpers.test.ts @@ -166,11 +166,14 @@ describe('NodeHelpers', () => { ).toEqual(false); }); it("should return false if node's rule rejects outgoing target", () => { + // Should not return false if the target is moving within the same parent expect( - helper( - 'canvas-node-reject-outgoing-dnd' - ).isDroppable('fixed-child-node', (err) => console.log(err)) + helper('canvas-node-reject-outgoing-dnd').isDroppable( + 'fixed-child-node' + ) ).toEqual(true); + + // should return false if the target moved to a different parent expect(helper('canvas-node').isDroppable('fixed-child-node')).toEqual( false );