Fix menu reordering in navigation (#1871)

src/navigation/components/MenuDetailsPage/MenuDetailsPage.tsx

@@ -12,12 +12,11 @@ import { Backlink } from "@saleor/macaw-ui";
 import React from "react";
 import { FormattedMessage, useIntl } from "react-intl";
 
-import { maybe } from "../../../misc";
 import { MenuDetails_menu } from "../../types/MenuDetails";
 import { MenuItemType } from "../MenuItemDialog";
 import MenuItems, { TreeOperation } from "../MenuItems";
 import MenuProperties from "../MenuProperties";
-import { computeTree } from "./tree";
+import { computeRelativeTree } from "./tree";
 
 export interface MenuDetailsFormData {
   name: string;
@@ -55,17 +54,20 @@ const MenuDetailsPage: React.FC<MenuDetailsPageProps> = ({
   const intl = useIntl();
 
   const initialForm: MenuDetailsFormData = {
-    name: maybe(() => menu.name, "")
+    name: menu?.name ?? ""
   };
 
   const [treeOperations, setTreeOperations] = React.useState<TreeOperation[]>(
     []
   );
 
+  const removeSimulatedMoves = (operations: TreeOperation[]) =>
+    operations.filter(operation => !operation.simulatedMove);
+
   const handleSubmit = async (data: MenuDetailsFormData) => {
     const result = await onSubmit({
       name: data.name,
-      operations: treeOperations
+      operations: removeSimulatedMoves(treeOperations)
     });
 
     if (result) {
@@ -75,10 +77,8 @@ const MenuDetailsPage: React.FC<MenuDetailsPageProps> = ({
     return result;
   };
 
-  const handleChange = (operation: TreeOperation) => {
+  const handleChange = (operations: TreeOperation[]) => {
-    if (!!operation) {
+    setTreeOperations([...treeOperations, ...operations]);
-      setTreeOperations([...treeOperations, operation]);
-    }
   };
 
   return (
@@ -110,17 +110,25 @@ const MenuDetailsPage: React.FC<MenuDetailsPageProps> = ({
               <CardSpacer />
               <MenuItems
                 canUndo={treeOperations.length > 0}
-                items={maybe(() =>
+                items={
-                  computeTree(menu.items, [...treeOperations])
+                  menu?.items
-                )}
+                    ? computeRelativeTree(menu.items, treeOperations)
+                    : []
+                }
                 onChange={handleChange}
                 onItemAdd={onItemAdd}
                 onItemClick={onItemClick}
                 onItemEdit={onItemEdit}
                 onUndo={() =>
-                  setTreeOperations(
+                  setTreeOperations(operations => {
-                    treeOperations.slice(0, treeOperations.length - 1)
+                    if (operations.length > 1) {
-                  )
+                      // Undo of a simulated move needs removal of 2 moves instead of one
+                      if (operations[operations.length - 2].simulatedMove) {
+                        return operations.slice(0, operations.length - 2);
+                      }
+                    }
+                    return operations.slice(0, operations.length - 1);
+                  })
                 }
               />
             </div>

src/navigation/components/MenuDetailsPage/tree.ts

@@ -45,23 +45,28 @@ function removeNode(
   return newTree;
 }
 
-function insertNode(
+function insertNode({
-  tree: MenuDetails_menu_items[],
+  tree,
-  path: number[],
+  path,
-  node: MenuDetails_menu_items,
+  node,
-  position: number
+  position
-): MenuDetails_menu_items[] {
+}: {
+  tree: MenuDetails_menu_items[];
+  path: number[];
+  node: MenuDetails_menu_items;
+  position: number;
+}): MenuDetails_menu_items[] {
   if (path.length === 0) {
     return [...tree.slice(0, position), node, ...tree.slice(position)];
   }
 
   if (path[0] in tree) {
-    tree[path[0]].children = insertNode(
+    tree[path[0]].children = insertNode({
-      tree[path[0]].children,
+      tree: tree[path[0]].children,
-      path.slice(1),
+      path: path.slice(1),
       node,
       position
-    );
+    });
   }
   return tree;
 }
@@ -89,45 +94,48 @@ function removeNodeAndChildren(
   return removeNode(tree, sourcePath);
 }
 
-function permuteNode(
+function permuteRelativeNode(
   tree: MenuDetails_menu_items[],
   permutation: TreeOperation
 ): MenuDetails_menu_items[] {
   const sourcePath = findNode(tree, permutation.id);
   const node = getNode(tree, sourcePath);
 
+  const hasParent = !!permutation.parentId;
+
   const treeAfterRemoval = removeNode(tree, sourcePath);
 
-  const targetPath = permutation.parentId
+  const targetPath = hasParent
     ? findNode(treeAfterRemoval, permutation.parentId)
     : [];
 
-  const treeAfterInsertion = insertNode(
+  const position = sourcePath[sourcePath.length - 1];
-    treeAfterRemoval,
+
-    targetPath,
+  const treeAfterInsertion = insertNode({
+    tree: treeAfterRemoval,
+    path: targetPath,
     node,
-    permutation.sortOrder
+    position: position + permutation.sortOrder
-  );
+  });
 
   return treeAfterInsertion;
 }
 
-function executeOperation(
+function executeRelativeOperation(
   tree: MenuDetails_menu_items[],
   operation: TreeOperation
 ): MenuDetails_menu_items[] {
   return operation.type === "move"
-    ? permuteNode(tree, operation)
+    ? permuteRelativeNode(tree, operation)
     : removeNodeAndChildren(tree, operation);
 }
 
-export function computeTree(
+export function computeRelativeTree(
   tree: MenuDetails_menu_items[],
   operations: TreeOperation[]
 ) {
   const newTree = operations.reduce(
-    (acc, operation) => executeOperation(acc, operation),
+    (acc, operation) => executeRelativeOperation(acc, operation),
-    // FIXME: 😡
     JSON.parse(JSON.stringify(tree))
   );
   return newTree;

src/navigation/components/MenuItems/MenuItems.tsx

@@ -21,7 +21,7 @@ const NODE_MARGIN = 40;
 export interface MenuItemsProps {
   canUndo: boolean;
   items: MenuDetails_menu_items[];
-  onChange: (operation: TreeOperation) => void;
+  onChange: (operations: TreeOperation[]) => void;
   onItemAdd: () => void;
   onItemClick: (id: string, type: MenuItemType) => void;
   onItemEdit: (id: string) => void;
@@ -181,10 +181,12 @@ const Node: React.FC<NodeRendererProps> = props => {
           className={classes.deleteButton}
           variant="secondary"
           onClick={() =>
-            node.onChange({
+            node.onChange([
-              id: node.id as any,
+              {
+                id: node.id,
                 type: "remove"
-            })
+              }
+            ])
           }
         >
           <DeleteIcon />
@@ -245,6 +247,7 @@ const MenuItems: React.FC<MenuItemsProps> = props => {
                 marginLeft: NODE_MARGIN * (path.length - 1)
               }
             })}
+            maxDepth={5}
             isVirtualized={false}
             rowHeight={NODE_HEIGHT}
             treeData={items.map(item =>

src/navigation/components/MenuItems/__snapshots__/tree.test.ts.snap

@@ -1,28 +1,34 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
 exports[`Properly computes diffs # 1`] = `
-Object {
+Array [
+  Object {
     "id": "1glasses",
     "parentId": "0jewelry",
     "sortOrder": 0,
     "type": "move",
-}
+  },
+]
 `;
 
 exports[`Properly computes diffs # 2`] = `
-Object {
+Array [
+  Object {
     "id": "1glasses",
     "parentId": "2accessories",
-  "sortOrder": 0,
+    "sortOrder": -1,
     "type": "move",
-}
+  },
+]
 `;
 
 exports[`Properly computes diffs # 3`] = `
-Object {
+Array [
+  Object {
     "id": "2accessories",
     "parentId": "4apparel",
     "sortOrder": 0,
     "type": "move",
-}
+  },
+]
 `;

src/navigation/components/MenuItems/tree.ts

@@ -10,6 +10,7 @@ export interface TreeOperation {
   type: TreeOperationType;
   parentId?: string;
   sortOrder?: number;
+  simulatedMove?: boolean;
 }
 
 export const unknownTypeError = Error("Unknown type");
@@ -64,11 +65,11 @@ export function getItemId(item: MenuDetails_menu_items): string {
 export function getDiff(
   originalTree: TreeItem[],
   newTree: TreeItem[]
-): TreeOperation {
+): TreeOperation[] {
   const originalMap = treeToMap(originalTree, "root");
   const newMap = treeToMap(newTree, "root");
 
-  const diff: TreeOperation[] = Object.keys(newMap).map(key => {
+  const diff: TreeOperation[] = Object.keys(newMap).flatMap(key => {
     const originalNode = originalMap[key];
     const newNode = newMap[key];
 
@@ -76,23 +77,56 @@ export function getDiff(
 
     if (patch.length > 0) {
       const addedNode = patch.find(operation => operation.type === "add");
+      const removedNode = patch.find(operation => operation.type === "remove");
+
       if (!!addedNode) {
+        const changedParent = originalNode.length !== newNode.length;
+        const sortOrder = removedNode
+          ? addedNode.newPos - removedNode.oldPos
+          : addedNode.newPos;
+
+        // This exists because backend doesn't recongize the position of the new node
+        // when it's moved from child to parent and/or up
+        // We have to make an additional move so that backend can sort the new tree correctly
+        // because without it the new node goes to the end of the parent node array by default
+        // SimulatedMove is removed before submit
+        if (changedParent && sortOrder !== originalNode.length) {
+          return [
+            {
+              id: addedNode.items[0],
+              parentId: key === "root" ? undefined : key,
+              sortOrder: newNode.length - 1,
+              type: "move" as TreeOperationType,
+              simulatedMove: true
+            },
+            {
+              id: addedNode.items[0],
+              parentId: key === "root" ? undefined : key,
+              sortOrder:
+                sortOrder - newNode.length < 0
+                  ? sortOrder - newNode.length + 1
+                  : sortOrder - newNode.length - 1,
+              type: "move" as TreeOperationType
+            }
+          ];
+        }
+
         return {
           id: addedNode.items[0],
           parentId: key === "root" ? undefined : key,
-          sortOrder: addedNode.newPos,
+          sortOrder,
           type: "move" as TreeOperationType
         };
       }
     }
   });
 
-  return diff.find(d => !!d);
+  return diff.filter(d => !!d);
 }
 
 export function getNodeData(
   item: MenuDetails_menu_items,
-  onChange: (operation: TreeOperation) => void,
+  onChange: (operations: TreeOperation[]) => void,
   onClick: (id: string, type: MenuItemType) => void,
   onEdit: (id: string) => void
 ): TreeItem {

src/storybook/__snapshots__/Stories.test.ts.snap

@@ -103589,14 +103589,23 @@ exports[`Storyshots Views / Navigation / Menu details loading 1`] = `
             </div>
             <div
               class="MenuItems-container-id"
-              style="min-height:56px;padding:0 24px;padding-top:20px"
+              style="min-height:-56px;padding:;padding-top:"
             >
-              <span
+              <div
-                class="Skeleton-skeleton-id"
+                class="rst__tree MenuItems-root-id"
-                data-test-id="skeleton"
               >
-                ‌
+                <div>
-              </span>
+                  <div
+                    class="MuiPaper-root-id MenuItems-row-id MuiPaper-elevation0-id MuiPaper-rounded-id"
+                  >
+                    <div
+                      class="MuiTypography-root-id MuiTypography-body1-id"
+                    >
+                      Add new menu item to begin creating menu
+                    </div>
+                  </div>
+                </div>
+              </div>
             </div>
             <div
               class="MuiCardActions-root-id MenuItems-actions-id MuiCardActions-spacing-id"