From 91d46b35295b45bb2f6e0f633684c04eb4005c1e Mon Sep 17 00:00:00 2001 From: Yoel Date: Tue, 29 Jan 2019 17:08:17 -0700 Subject: [PATCH] move_node tests and code clean up (#2555) * tests * refactor moveNodeByPath * refactor element.move_node * refactor invert.move_node --- packages/slate/src/commands/by-path.js | 18 ++++++--- packages/slate/src/interfaces/element.js | 5 +-- packages/slate/src/operations/invert.js | 29 +++----------- .../move-node-by-path/new-path-exists.js | 38 ++++++++++++++++++ .../new-sibling-is-text-node.js | 34 ++++++++++++++++ .../path-left-sibling-of-new-path-ancestor.js | 39 +++++++++++++++++++ .../by-path/move-node-by-path/sibling-swap.js | 39 +++++++++++++++++++ .../by-path/move-node-by-path/text-nodes.js | 35 +++++++++++++++++ ...move-node-by-path-ancestor-left-sibling.js | 27 +++++++++++++ .../test/history/undo/move-node-by-path.js | 27 +++++++++++++ 10 files changed, 258 insertions(+), 33 deletions(-) create mode 100644 packages/slate/test/commands/by-path/move-node-by-path/new-path-exists.js create mode 100644 packages/slate/test/commands/by-path/move-node-by-path/new-sibling-is-text-node.js create mode 100644 packages/slate/test/commands/by-path/move-node-by-path/path-left-sibling-of-new-path-ancestor.js create mode 100644 packages/slate/test/commands/by-path/move-node-by-path/sibling-swap.js create mode 100644 packages/slate/test/commands/by-path/move-node-by-path/text-nodes.js create mode 100644 packages/slate/test/history/undo/move-node-by-path-ancestor-left-sibling.js create mode 100644 packages/slate/test/history/undo/move-node-by-path.js diff --git a/packages/slate/src/commands/by-path.js b/packages/slate/src/commands/by-path.js index 5333e7aef..8dc71b9e9 100644 --- a/packages/slate/src/commands/by-path.js +++ b/packages/slate/src/commands/by-path.js @@ -183,19 +183,25 @@ Commands.mergeNodeByPath = (editor, path) => { } /** - * Move a node by `path` to a new parent by `newPath` and `index`. + * Move a node by `path` to a new parent by `newParentPath` and `newIndex`. * * @param {Editor} editor * @param {Array} path - * @param {String} newPath - * @param {Number} index + * @param {String} newParentPath + * @param {Number} newIndex */ -Commands.moveNodeByPath = (editor, path, newPath, newIndex) => { +Commands.moveNodeByPath = (editor, path, newParentPath, newIndex) => { const { value } = editor - // If the operation path and newPath are the same, + // If the operation path and newParentPath are the same, // this should be considered a NOOP + if (PathUtils.isEqual(path, newParentPath)) { + return editor + } + + const newPath = newParentPath.concat(newIndex) + if (PathUtils.isEqual(path, newPath)) { return editor } @@ -204,7 +210,7 @@ Commands.moveNodeByPath = (editor, path, newPath, newIndex) => { type: 'move_node', value, path, - newPath: newPath.concat(newIndex), + newPath, }) } diff --git a/packages/slate/src/interfaces/element.js b/packages/slate/src/interfaces/element.js index e2d76c2fd..8f590c3cc 100644 --- a/packages/slate/src/interfaces/element.js +++ b/packages/slate/src/interfaces/element.js @@ -1763,13 +1763,12 @@ class ElementInterface { const newParentPath = PathUtils.lift(newPath) this.assertNode(newParentPath) - const [p, np] = PathUtils.crop(path, newPath) - const position = PathUtils.compare(p, np) + const position = PathUtils.compare(path, newPath) // If the old path ends above and before a node in the new path, then // removing it will alter the target, so we need to adjust the new path. if (path.size < newPath.size && position === -1) { - newPath = PathUtils.decrement(newPath, 1, p.size - 1) + newPath = PathUtils.decrement(newPath, 1, path.size - 1) } let ret = this diff --git a/packages/slate/src/operations/invert.js b/packages/slate/src/operations/invert.js index 781a18dbd..1c1c34a20 100644 --- a/packages/slate/src/operations/invert.js +++ b/packages/slate/src/operations/invert.js @@ -45,36 +45,17 @@ function invertOperation(op) { let inversePath = newPath let inverseNewPath = path - const pathLast = path.size - 1 - const newPathLast = newPath.size - 1 + const position = PathUtils.compare(path, newPath) // If the node's old position was a left sibling of an ancestor of // its new position, we need to adjust part of the path by -1. - if ( - path.size < inversePath.size && - path.slice(0, pathLast).every((e, i) => e == inversePath.get(i)) && - path.last() < inversePath.get(pathLast) - ) { - inversePath = inversePath - .slice(0, pathLast) - .concat(inversePath.get(pathLast) - 1) - .concat(inversePath.slice(pathLast + 1, inversePath.size)) - } - // If the node's new position is an ancestor of the old position, // or a left sibling of an ancestor of its old position, we need // to adjust part of the path by 1. - if ( - newPath.size < inverseNewPath.size && - newPath - .slice(0, newPathLast) - .every((e, i) => e == inverseNewPath.get(i)) && - newPath.last() <= inverseNewPath.get(newPathLast) - ) { - inverseNewPath = inverseNewPath - .slice(0, newPathLast) - .concat(inverseNewPath.get(newPathLast) + 1) - .concat(inverseNewPath.slice(newPathLast + 1, inverseNewPath.size)) + if (path.size < newPath.size && position === -1) { + inversePath = PathUtils.decrement(newPath, 1, path.size - 1) + } else if (path.size > newPath.size && position !== -1) { + inverseNewPath = PathUtils.increment(path, 1, newPath.size - 1) } const inverse = op.set('path', inversePath).set('newPath', inverseNewPath) diff --git a/packages/slate/test/commands/by-path/move-node-by-path/new-path-exists.js b/packages/slate/test/commands/by-path/move-node-by-path/new-path-exists.js new file mode 100644 index 000000000..97f503b4b --- /dev/null +++ b/packages/slate/test/commands/by-path/move-node-by-path/new-path-exists.js @@ -0,0 +1,38 @@ +/** @jsx h */ + +import h from '../../../helpers/h' +import PathUtils from '../../../../src/utils/path-utils' +import assert from 'assert' + +const pathA = PathUtils.create([0, 0]) +const pathB = PathUtils.create([1]) + +export default function(editor) { + editor.moveNodeByPath(pathA, pathB, 0) + assert(editor.operations.size >= 1) +} + +export const input = ( + + + + I am gonna move + + + I am an existing node at newPath + + + +) + +export const output = ( + + + + + I am gonna move + I am an existing node at newPath + + + +) diff --git a/packages/slate/test/commands/by-path/move-node-by-path/new-sibling-is-text-node.js b/packages/slate/test/commands/by-path/move-node-by-path/new-sibling-is-text-node.js new file mode 100644 index 000000000..507c03bb4 --- /dev/null +++ b/packages/slate/test/commands/by-path/move-node-by-path/new-sibling-is-text-node.js @@ -0,0 +1,34 @@ +/** @jsx h */ + +import h from '../../../helpers/h' +import PathUtils from '../../../../src/utils/path-utils' +import assert from 'assert' + +const pathA = PathUtils.create([0]) +const pathB = PathUtils.create([1]) + +export default function(editor) { + editor.moveNodeByPath(pathA, pathB, 1) + assert(editor.operations.size >= 1) +} + +export const input = ( + + + I am gonna move + + Existing text + + + +) + +export const output = ( + + + + Existing textI am gonna move + + + +) diff --git a/packages/slate/test/commands/by-path/move-node-by-path/path-left-sibling-of-new-path-ancestor.js b/packages/slate/test/commands/by-path/move-node-by-path/path-left-sibling-of-new-path-ancestor.js new file mode 100644 index 000000000..77515b313 --- /dev/null +++ b/packages/slate/test/commands/by-path/move-node-by-path/path-left-sibling-of-new-path-ancestor.js @@ -0,0 +1,39 @@ +/** @jsx h */ + +import h from '../../../helpers/h' +import PathUtils from '../../../../src/utils/path-utils' +import assert from 'assert' + +const pathA = PathUtils.create([0]) +const pathB = PathUtils.create([1]) + +export default function(editor) { + editor.moveNodeByPath(pathA, pathB, 1) + assert(editor.operations.size >= 1) +} + +export const input = ( + + + + I am gonna move + + + I am an existing node in newParent + + + +) + +export const output = ( + + + + I am an existing node in newParent + + I am gonna move + + + + +) diff --git a/packages/slate/test/commands/by-path/move-node-by-path/sibling-swap.js b/packages/slate/test/commands/by-path/move-node-by-path/sibling-swap.js new file mode 100644 index 000000000..f9d044593 --- /dev/null +++ b/packages/slate/test/commands/by-path/move-node-by-path/sibling-swap.js @@ -0,0 +1,39 @@ +/** @jsx h */ + +import h from '../../../helpers/h' +import PathUtils from '../../../../src/utils/path-utils' +import assert from 'assert' + +const pathA = PathUtils.create([0, 0]) +const pathB = PathUtils.create([0]) + +export default function(editor) { + editor.moveNodeByPath(pathA, pathB, 1) + assert(editor.operations.size >= 1) +} + +export const input = ( + + + + + I am gonna move + + I am an existing node at newPath + + + +) + +export const output = ( + + + + I am an existing node at newPath + + I am gonna move + + + + +) diff --git a/packages/slate/test/commands/by-path/move-node-by-path/text-nodes.js b/packages/slate/test/commands/by-path/move-node-by-path/text-nodes.js new file mode 100644 index 000000000..1266663ab --- /dev/null +++ b/packages/slate/test/commands/by-path/move-node-by-path/text-nodes.js @@ -0,0 +1,35 @@ +/** @jsx h */ + +import h from '../../../helpers/h' +import PathUtils from '../../../../src/utils/path-utils' +import assert from 'assert' + +const pathA = PathUtils.create([0, 0]) +const pathB = PathUtils.create([1]) + +export default function(editor) { + editor.moveNodeByPath(pathA, pathB, 1) + assert(editor.operations.size >= 1) +} + +export const input = ( + + + Text that will move + + Existing text + + + +) + +export const output = ( + + + + + Existing textText that will move + + + +) diff --git a/packages/slate/test/history/undo/move-node-by-path-ancestor-left-sibling.js b/packages/slate/test/history/undo/move-node-by-path-ancestor-left-sibling.js new file mode 100644 index 000000000..634dcc445 --- /dev/null +++ b/packages/slate/test/history/undo/move-node-by-path-ancestor-left-sibling.js @@ -0,0 +1,27 @@ +/** @jsx h */ + +import h from '../../helpers/h' +import PathUtils from '../../../src/utils/path-utils' + +const pathA = PathUtils.create([0]) +const pathB = PathUtils.create([1]) + +export default function(editor) { + editor.moveNodeByPath(pathA, pathB, 1) + editor.flush().undo() +} + +export const input = ( + + + + I am gonna move + + + I am an existing node in newParent + + + +) + +export const output = input diff --git a/packages/slate/test/history/undo/move-node-by-path.js b/packages/slate/test/history/undo/move-node-by-path.js new file mode 100644 index 000000000..c54a0b44f --- /dev/null +++ b/packages/slate/test/history/undo/move-node-by-path.js @@ -0,0 +1,27 @@ +/** @jsx h */ + +import h from '../../helpers/h' +import PathUtils from '../../../src/utils/path-utils' + +const pathA = PathUtils.create([0, 0]) +const pathB = PathUtils.create([1]) + +export default function(editor) { + editor.moveNodeByPath(pathA, pathB, 1) + editor.flush().undo() +} + +export const input = ( + + + + I am gonna move + + + I am an existing node at newPath + + + +) + +export const output = input