Fix editable tree item bugs (#3187)

mui · Feb 9, 2024 · 1c6a90d · 1c6a90d
1 parent 7acbb93
commit 1c6a90d
Show file tree

Hide file tree

Showing 6 changed files with 82 additions and 61 deletions.
diff --git a/packages/toolpad-app/src/components/EditableTreeItem.tsx b/packages/toolpad-app/src/components/EditableTreeItem.tsx
@@ -81,10 +81,10 @@ export default function EditableTreeItem({
       handleCancel();
       return;
     }
+
     if (onEdit) {
       onEdit(itemNameInput);
     }
-    setItemNameInput('');
     setIsInternalEditing(false);
   }, [handleCancel, itemNameInput, newItemValidationResult.isValid, onEdit]);
 

diff --git a/packages/toolpad-app/src/toolpad/AppEditor/PagesExplorer/index.tsx b/packages/toolpad-app/src/toolpad/AppEditor/PagesExplorer/index.tsx
@@ -1,5 +1,5 @@
 import * as React from 'react';
-import { styled, Box, IconButton, Stack } from '@mui/material';
+import { styled, Box, IconButton, Stack, Tooltip } from '@mui/material';
 import { TreeView, treeItemClasses } from '@mui/x-tree-view';
 import ExpandMoreIcon from '@mui/icons-material/ExpandMore';
 import ChevronRightIcon from '@mui/icons-material/ChevronRight';
@@ -10,7 +10,7 @@ import invariant from 'invariant';
 import { alphabeticComparator, createPropComparator } from '@mui/toolpad-utils/comparators';
 import useBoolean from '@mui/toolpad-utils/hooks/useBoolean';
 import * as appDom from '@mui/toolpad-core/appDom';
-import { useAppStateApi, useAppState, useDomApi } from '../../AppState';
+import { useAppStateApi, useAppState } from '../../AppState';
 import useLocalStorageState from '../../../utils/useLocalStorageState';
 import NodeMenu from '../NodeMenu';
 import { DomView } from '../../../utils/domView';
@@ -61,6 +61,7 @@ function PagesExplorerTreeItem(props: StyledTreeItemProps) {
     nodeId,
     labelIcon,
     labelText,
+    title,
     onRenameNode,
     onDeleteNode,
     onDuplicateNode,
@@ -99,33 +100,35 @@ function PagesExplorerTreeItem(props: StyledTreeItemProps) {
       nodeId={nodeId}
       labelText={labelText}
       renderLabel={(children) => (
-        <Box sx={{ display: 'flex', alignItems: 'center' }}>
-          {labelIcon}
-          {children}
-          {toolpadNodeId ? (
-            <NodeMenu
-              renderButton={({ buttonProps, menuProps }) => (
-                <IconButton
-                  className={clsx(classes.treeItemMenuButton, {
-                    [classes.treeItemMenuOpen]: menuProps.open,
-                  })}
-                  aria-label="Open page explorer menu"
-                  size="small"
-                  {...buttonProps}
-                >
-                  <MoreVertIcon fontSize="inherit" />
-                </IconButton>
-              )}
-              nodeId={toolpadNodeId}
-              renameLabelText={renameLabelText}
-              deleteLabelText={deleteLabelText}
-              duplicateLabelText={duplicateLabelText}
-              onRenameNode={startEditing}
-              onDeleteNode={onDeleteNode}
-              onDuplicateNode={onDuplicateNode}
-            />
-          ) : null}
-        </Box>
+        <Tooltip title={title} placement="right" disableInteractive>
+          <Box sx={{ display: 'flex', alignItems: 'center' }}>
+            {labelIcon}
+            {children}
+            {toolpadNodeId ? (
+              <NodeMenu
+                renderButton={({ buttonProps, menuProps }) => (
+                  <IconButton
+                    className={clsx(classes.treeItemMenuButton, {
+                      [classes.treeItemMenuOpen]: menuProps.open,
+                    })}
+                    aria-label="Open page explorer menu"
+                    size="small"
+                    {...buttonProps}
+                  >
+                    <MoreVertIcon fontSize="inherit" />
+                  </IconButton>
+                )}
+                nodeId={toolpadNodeId}
+                renameLabelText={renameLabelText}
+                deleteLabelText={deleteLabelText}
+                duplicateLabelText={duplicateLabelText}
+                onRenameNode={startEditing}
+                onDeleteNode={onDeleteNode}
+                onDuplicateNode={onDuplicateNode}
+              />
+            ) : null}
+          </Box>
+        </Tooltip>
       )}
       suggestedNewItemName={labelText}
       onCancel={stopEditing}
@@ -155,7 +158,6 @@ export interface PagesExplorerProps {
 export default function PagesExplorer({ className }: PagesExplorerProps) {
   const projectApi = useProjectApi();
   const { dom, currentView } = useAppState();
-  const domApi = useDomApi();
   const appStateApi = useAppStateApi();
 
   const app = appDom.getApp(dom);
@@ -276,23 +278,30 @@ export default function PagesExplorer({ className }: PagesExplorerProps) {
       if (nodeId === activePage?.id) {
         const siblings = appDom.getSiblings(dom, deletedNode);
         const firstSiblingOfType = siblings.find((sibling) => sibling.type === deletedNode.type);
-        domViewAfterDelete = firstSiblingOfType && getNodeEditorDomView(firstSiblingOfType);
+        domViewAfterDelete = firstSiblingOfType
+          ? getNodeEditorDomView(firstSiblingOfType)
+          : { kind: 'page' };
       }
 
       await projectApi.methods.deletePage(deletedNode.name);
 
-      appStateApi.update(
-        (draft) => appDom.removeNode(draft, nodeId),
-        domViewAfterDelete || { kind: 'page' },
-      );
+      appStateApi.update((draft) => appDom.removeNode(draft, nodeId), domViewAfterDelete);
     },
     [projectApi, activePage?.id, appStateApi, dom],
   );
 
   const handleRenameNode = React.useCallback(
     (nodeId: NodeId, updatedName: string) => {
-      domApi.setNodeName(nodeId, updatedName);
-      appStateApi.setView({ kind: 'page', name: updatedName });
+      appStateApi.update(
+        (draft) => {
+          const page = appDom.getNode(draft, nodeId, 'page');
+          return appDom.setNodeName(draft, page, updatedName);
+        },
+        {
+          kind: 'page',
+          name: updatedName,
+        },
+      );
 
       const oldNameNode = dom.nodes[nodeId];
       if (oldNameNode.type === 'page' && updatedName !== oldNameNode.name) {
@@ -301,7 +310,7 @@ export default function PagesExplorer({ className }: PagesExplorerProps) {
         }, 300);
       }
     },
-    [projectApi, dom.nodes, domApi, appStateApi],
+    [appStateApi, dom.nodes, projectApi.methods],
   );
 
   const handleDuplicateNode = React.useCallback(
@@ -368,8 +377,8 @@ export default function PagesExplorer({ className }: PagesExplorerProps) {
             key={page.id}
             nodeId={page.id}
             toolpadNodeId={page.id}
-            labelText={appDom.getPageDisplayName(page)}
-            title={page.name}
+            labelText={page.name}
+            title={appDom.getPageDisplayName(page)}
             onRenameNode={handleRenameNode}
             onDuplicateNode={handleDuplicateNode}
             onDeleteNode={handleDeletePage}

diff --git a/test/integration/duplication/index.spec.ts b/test/integration/duplication/index.spec.ts
@@ -19,7 +19,7 @@ test('duplication', async ({ page }) => {
   await editorModel.goto();
 
   {
-    await editorModel.openPageExplorerMenu('Page 1');
+    await editorModel.openPageExplorerMenu('page1');
     const duplicateMenuItem = page.getByRole('menuitem', { name: 'Duplicate' });
     await duplicateMenuItem.click();
 
@@ -28,12 +28,12 @@ test('duplication', async ({ page }) => {
     const button = editorModel.appCanvas.getByRole('button', { name: 'hello world' });
     await expect(button).toBeVisible();
 
-    await editorModel.openPageExplorerMenu('Page 2');
+    await editorModel.openPageExplorerMenu('page2');
     const deleteMenuItem = page.getByRole('menuitem', { name: 'Delete' });
     await deleteMenuItem.click();
     const deleteButton = editorModel.confirmationDialog.getByRole('button', { name: 'Delete' });
     await deleteButton.click();
 
-    await expect(editorModel.getExplorerItem('Page 2')).toBeHidden();
+    await expect(editorModel.getExplorerItem('page2')).toBeHidden();
   }
 });
diff --git a/test/integration/editor/new.spec.ts b/test/integration/editor/new.spec.ts
@@ -1,7 +1,7 @@
 import path from 'path';
 import invariant from 'invariant';
 import { fileExists, folderExists } from '@mui/toolpad-utils/fs';
-import { test, expect } from '../../playwright/localTest';
+import { test, expect, Locator } from '../../playwright/localTest';
 import { ToolpadEditor } from '../../models/ToolpadEditor';
 
 test.use({
@@ -63,27 +63,39 @@ test('can create/delete page', async ({ page, localApp }) => {
   await editorModel.goto();
 
   await editorModel.createPage('someOtherPage');
+  await editorModel.createPage('andOneMorePage');
 
-  const pageMenuItem = editorModel.getExplorerItem('Some Other Page');
-  const pageFolder = path.resolve(localApp.dir, './toolpad/pages/someOtherPage');
-  const pageFile = path.resolve(pageFolder, './page.yml');
+  const deletePageFromExplorer = async (pageMenuItem: Locator) => {
+    await pageMenuItem.hover();
 
-  await expect(pageMenuItem).toBeVisible();
-  await expect.poll(async () => folderExists(pageFolder)).toBe(true);
-  await expect.poll(async () => fileExists(pageFile)).toBe(true);
+    await pageMenuItem.getByRole('button', { name: 'Open page explorer menu' }).click();
+
+    await page.getByRole('menuitem', { name: 'Delete' }).click();
+
+    await page
+      .getByRole('dialog', { name: 'Confirm' })
+      .getByRole('button', { name: 'Delete' })
+      .click();
+  };
 
-  await pageMenuItem.hover();
+  // Delete another page
 
-  await pageMenuItem.getByRole('button', { name: 'Open page explorer menu' }).click();
+  const anotherPageMenuItem = editorModel.getExplorerItem('someOtherPage');
+  await deletePageFromExplorer(anotherPageMenuItem);
+  await expect(anotherPageMenuItem).toBeHidden();
 
-  await page.getByRole('menuitem', { name: 'Delete' }).click();
+  // Delete current page
 
-  await page
-    .getByRole('dialog', { name: 'Confirm' })
-    .getByRole('button', { name: 'Delete' })
-    .click();
+  const currentPageMenuItem = editorModel.getExplorerItem('andOneMorePage');
+  const pageFolder = path.resolve(localApp.dir, './toolpad/pages/andOneMorePage');
+  const pageFile = path.resolve(pageFolder, './page.yml');
+
+  await expect(currentPageMenuItem).toBeVisible();
+  await expect.poll(async () => folderExists(pageFolder)).toBe(true);
+  await expect.poll(async () => fileExists(pageFile)).toBe(true);
 
-  await expect(pageMenuItem).toBeHidden();
+  await deletePageFromExplorer(currentPageMenuItem);
+  await expect(currentPageMenuItem).toBeHidden();
 
   await expect.poll(async () => folderExists(pageFolder)).toBe(false);
 });
diff --git a/test/integration/pages/index.spec.ts b/test/integration/pages/index.spec.ts
@@ -56,7 +56,7 @@ test('can rename page', async ({ page, localApp }) => {
   const oldPageFolder = path.resolve(localApp.dir, './toolpad/pages/page2');
   await expect.poll(async () => folderExists(oldPageFolder)).toBe(true);
 
-  await editorModel.explorer.getByText('Page 2').dblclick();
+  await editorModel.explorer.getByText('page2').dblclick();
   await page.keyboard.type('renamedpage');
   await page.keyboard.press('Enter');
 

diff --git a/test/integration/undo-redo/multiple-pages.spec.ts b/test/integration/undo-redo/multiple-pages.spec.ts
@@ -25,7 +25,7 @@ test('test undo and redo through different pages', async ({ page }) => {
   });
   await expect(pageButton1).toBeVisible();
 
-  await editorModel.explorer.getByText('Page 2').click();
+  await editorModel.explorer.getByText('page2').click();
 
   const pageButton2 = editorModel.appCanvas.getByRole('button', {
     name: 'page2Button',