more tests & fix a bug

apps/designer/app/designer/shared/tree/tree.test.ts

@@ -5,28 +5,88 @@ import { createInstance } from "~/shared/tree-utils";
 import { useHorizontalShift } from "./tree";
 
 describe("useHorizontalShift", () => {
-  const box1 = createInstance({ component: "Box" });
-  const box2 = createInstance({ component: "Box" });
-
-  const tree = createInstance({
+  const box1 = createInstance({ component: "Box", id: "box1" });
+  const box2 = createInstance({ component: "Box", id: "box2" });
+  const box321 = createInstance({ component: "Box", id: "box321" });
+  const box32 = createInstance({
     component: "Box",
-    children: [box1, box2],
+    children: [box321],
+    id: "box32",
+  });
+  const box31 = createInstance({ component: "Box", id: "box31" });
+  const box3 = createInstance({
+    component: "Box",
+    children: [box31, box32],
+    id: "box3",
+  });
+  const box4 = createInstance({ component: "Box", id: "box4" });
+  const heading = createInstance({
+    component: "Heading",
+    children: [createInstance({ component: "Bold" })],
+    id: "heading",
+  });
+  const box511 = createInstance({ component: "Box", id: "box511" });
+  const box51 = createInstance({
+    component: "Box",
+    id: "box51",
+    children: [box511],
+  });
+  const box52 = createInstance({ component: "Box", id: "box52" });
+  const box5 = createInstance({
+    component: "Box",
+    id: "box5",
+    children: [box51, box52],
   });
 
-  const makeDropTarget = (
-    instance: Instance,
-    indexWithinChildren: number,
-    placement: Placement
-  ): DropTarget<Instance> => ({
+  // Body
+  // | Box (box1)
+  // | Box (box2)
+  // | Box (box3)
+  // | | Box (box31)
+  // | | Box (box32)
+  // | | | Box (box321)
+  // | Box (box4)
+  // | Heading (heading)
+  // | | Bold
+  // | Box (box5)
+  // | | Box (box51)
+  // | | | Box (box511)
+  // | | Box (box52)
+  const tree = createInstance({
+    id: "root",
+    component: "Body",
+    children: [box1, box2, box3, box4, heading, box5],
+  });
+
+  const makePlacement = (
+    type: Placement["type"] = "next-to-child"
+  ): Placement => ({
+    type,
+    direction: "horizontal",
+    x: 0,
+    y: 100,
+    length: 300,
+  });
+
+  const makeDrop = ({
+    into,
+    after,
+    placement = makePlacement(),
+  }: {
+    into: Instance;
+    after?: Instance;
+    placement?: Placement;
+  }): DropTarget<Instance> => ({
     element: null as unknown as HTMLElement, // element is not used
-    data: instance,
+    data: into,
     rect: {
       top: Math.random(),
       left: Math.random(),
       width: Math.random(),
       height: Math.random(),
     } as DOMRect,
-    indexWithinChildren,
+    indexWithinChildren:
+      after === undefined ? 0 : into.children.indexOf(after) + 1,
     placement,
   });
 
@@ -45,7 +105,7 @@ describe("useHorizontalShift", () => {
         dragItem,
         dropTarget,
         root: tree,
-        getIsExpanded: () => true,
+        getIsExpanded: (instance) => instance.children.length > 0,
       },
     });
 
@@ -54,37 +114,191 @@ describe("useHorizontalShift", () => {
       setHorizontalShift(shift);
     });
 
-    const [shiftedDropTarget] = result.current;
-
-    return shiftedDropTarget;
+    const [shiftedDroptTarget] = result.current;
+    return shiftedDroptTarget;
   };
 
   test("if there's no dragItem or dropTarget returns undefined", () => {
-    const shiftedDropTarget = render({
+    const result = render({
       dragItem: undefined,
       dropTarget: undefined,
     });
-    expect(shiftedDropTarget).toBeUndefined();
+    expect(result).toBeUndefined();
   });
 
   test("drop target is empty or collapsed", () => {
-    const dropTarget = makeDropTarget(box2, 0, {
-      type: "inside-parent",
-      direction: "horizontal",
-      x: 0,
-      y: 0,
-      length: 0,
+    const dropTarget = makeDrop({
+      into: box2,
+      placement: makePlacement("inside-parent"),
     });
-
-    const shiftedDropTarget = render({ dragItem: box1, dropTarget });
+    const result = render({ dragItem: box1, dropTarget });
 
     // parent doesn't change
-    expect(shiftedDropTarget?.instance).toEqual(box2);
+    expect(result?.instance.id).toBe(box2.id);
 
     // placement is the original rect
-    expect(shiftedDropTarget?.placement).toEqual({
+    expect(result?.placement).toEqual({
       type: "rect",
       rect: dropTarget.rect,
     });
   });
+
+  test("placement line coordinates are always adjusted", () => {
+    const result = render(
+      { dragItem: box31, dropTarget: makeDrop({ into: box3, after: box32 }) },
+      0
+    );
+
+    // parent doesn't change
+    expect(result?.instance.id).toBe(box3.id);
+
+    // placement line is adjusted to account for depth,
+    // even though depth didn't change
+    expect(result?.placement).toEqual({
+      type: "line",
+      placement: { ...makePlacement(), length: 260, x: 40 },
+    });
+  });
+
+  test("shifting is relative to the drag item's depth", () => {
+    // same shift and drop target
+    const shift = 0;
+    const dropTarget = makeDrop({ into: tree, after: box3 });
+
+    // box2's depth is 1, so it's inserted without shifting
+    const result1 = render({ dragItem: box2, dropTarget }, shift);
+    expect(result1?.instance.id).toBe(tree.id);
+
+    // box31's depth is 2, so it's inserted with shifting, to maintain the same depth
+    const result2 = render({ dragItem: box31, dropTarget }, shift);
+    expect(result2?.instance.id).toBe(box3.id);
+    expect(result2?.position).toBe("end");
+  });
+
+  describe("shifting to the left", () => {
+    test("if we're already at the root, no shift occurs", () => {
+      const result = render(
+        { dragItem: box1, dropTarget: makeDrop({ into: tree, after: box2 }) },
+        -1
+      );
+      expect(result?.instance.id).toBe(tree.id);
+    });
+
+    test("a shifting is possible only when we're at the bottom of the initial drop target", () => {
+      const result1 = render(
+        { dragItem: box1, dropTarget: makeDrop({ into: box3, after: box31 }) },
+        -1
+      );
+      expect(result1?.instance.id).toBe(box3.id);
+    });
+
+    test("when above the drag item, which is at the bottom, the shift is allowed", () => {
+      const result = render(
+        { dragItem: box32, dropTarget: makeDrop({ into: box3, after: box31 }) },
+        -1
+      );
+      expect(result?.instance.id).toBe(tree.id);
+      expect(result?.position).toBe(tree.children.indexOf(box3) + 1);
+      expect(result?.placement).toEqual({
+        type: "line",
+        placement: { ...makePlacement(), x: 24, length: 276 },
+      });
+    });
+
+    test("shifting by different ammounts works correctly", () => {
+      const args = {
+        dragItem: box1,
+        dropTarget: makeDrop({ into: box32, after: box321 }),
+      };
+
+      const result1 = render(args, 0);
+      expect(result1?.instance.id).toBe(tree.id);
+      expect(result1?.position).toBe(tree.children.indexOf(box3) + 1);
+      expect(result1?.placement).toEqual({
+        type: "line",
+        placement: { ...makePlacement(), x: 24, length: 276 },
+      });
+
+      const result2 = render(args, 1);
+      expect(result2?.instance.id).toBe(box3.id);
+      expect(result2?.position).toBe(box3.children.indexOf(box32) + 1);
+      expect(result2?.placement).toEqual({
+        type: "line",
+        placement: { ...makePlacement(), x: 40, length: 260 },
+      });
+    });
+
+    test('the "at the bottom" check is perforamed at every step, dissalowing futher shifting when we\'re no longer at the bottom', () => {
+      const result = render(
+        {
+          dragItem: box1,
+          dropTarget: makeDrop({ into: box51, after: box511 }),
+        },
+        -10
+      );
+      expect(result?.instance.id).toBe(box5.id);
+      expect(result?.position).toBe(box5.children.indexOf(box51) + 1);
+    });
+  });
+
+  describe("shifting to the right ", () => {
+    test("if there's no expanded item above, no shift occurs", () => {
+      const result = render(
+        { dragItem: box1, dropTarget: makeDrop({ into: tree, after: box2 }) },
+        1
+      );
+      expect(result?.instance.id).toBe(tree.id);
+    });
+
+    test("if there's an expanded item above, we shift inside it", () => {
+      const args = {
+        dragItem: box1,
+        dropTarget: makeDrop({ into: tree, after: box3 }),
+      };
+
+      const result1 = render(args, 1);
+      expect(result1?.instance.id).toBe(box3.id);
+
+      const result2 = render(args, 2);
+      expect(result2?.instance.id).toBe(box32.id);
+    });
+
+    test("you cannot move inside drag item by shifting", () => {
+      // drag item is right above, no shifting
+      const result1 = render(
+        { dragItem: box3, dropTarget: makeDrop({ into: tree, after: box3 }) },
+        3
+      );
+      expect(result1?.instance.id).toBe(tree.id);
+
+      // drag item is inside the item above
+      // we shift into the item above, but not inside the drag item
+      const result2 = render(
+        { dragItem: box32, dropTarget: makeDrop({ into: tree, after: box3 }) },
+        3
+      );
+      expect(result2?.instance.id).toBe(box3.id);
+      expect(result2?.position).toBe("end");
+    });
+
+    test("when the item above is the drag item itself, we consider the item above it as a potential target", () => {
+      const result = render(
+        { dragItem: box4, dropTarget: makeDrop({ into: tree, after: box4 }) },
+        1
+      );
+      expect(result?.instance.id).toBe(box3.id);
+      expect(result?.position).toBe("end");
+    });
+
+    test("if there's an expanded item above, but it cannot accept shildren, no shift occurs", () => {
+      const result = render(
+        {
+          dragItem: box1,
+          dropTarget: makeDrop({ into: tree, after: heading }),
+        },
+        1
+      );
+      expect(result?.instance.id).toBe(tree.id);
+    });
+  });
 });

apps/designer/app/designer/shared/tree/tree.tsx

@@ -571,36 +571,43 @@ export const useHorizontalShift = ({
       typeof instance === "object" && instance.id === dragItem.id;
 
     if (desiredDepth < currentDepth) {
-      // There's a special case when the placement line is above the drag item.
-      // For reparenting above and below the drag item means the same thing.
-      const indexCorrected = isDragItem(data.children[indexWithinChildren])
-        ? indexWithinChildren + 1
-        : indexWithinChildren;
+      let shifted = 0;
+      let newParent = data;
+      let newPosition = indexWithinChildren;
 
-      // Unless we're currently at the bottom of drop target's children,
-      // decreasing depth will not correspond to a new meaningful position
-      if (indexCorrected !== data.children.length) {
-        return withoutShift;
+      const isAtTheBottom = (parent: Instance, index: number) => {
+        // There's a special case when the placement line is above the drag item.
+        // For reparenting above and below the drag item means the same thing.
+        const indexCorrected = isDragItem(parent.children[index])
+          ? index + 1
+          : index;
+        return indexCorrected === parent.children.length;
+      };
+
+      let potentialNewParent = dropTargetPath[shifted + 1];
+
+      while (
+        isAtTheBottom(newParent, newPosition) &&
+        typeof potentialNewParent === "object" &&
+        components[potentialNewParent.instance.component].canAcceptChild() &&
+        shifted < currentDepth - desiredDepth
+      ) {
+        shifted++;
+        newPosition = dropTargetPath[shifted - 1].position + 1;
+        newParent = potentialNewParent.instance;
+        potentialNewParent = dropTargetPath[shifted + 1];
       }
 
-      const difference = Math.min(
-        dropTargetPath.length - 1,
-        currentDepth - desiredDepth
-      );
-
-      if (difference === 0) {
+      if (shifted === 0) {
         return withoutShift;
       }
 
-      // Ideally we should check canAcceptChildren on the new target
-      // but we assume that because it already has a child, it can accept more.
-
       return {
-        instance: dropTargetPath[difference].instance,
-        position: dropTargetPath[difference - 1].position + 1,
+        instance: newParent,
+        position: newPosition,
         placement: {
           type: "line",
-          placement: shiftPlacement(currentDepth - difference),
+          placement: shiftPlacement(currentDepth - shifted),
         },
       };
     }