Merge pull request #1466 from pablo-mayrgundter/fix-panel-drag

Fix SideDrawer drag

Author: pablo-mayrgundter

package.json

@@ -1,6 +1,6 @@
 {
   "name": "bldrs",
-  "version": "1.0.1890",
+  "version": "1.0.1894",
   "main": "src/index.jsx",
   "license": "AGPL-3.0",
   "homepage": "https://github.com/bldrs-ai/Share",

src/Components/Apps/Apps.spec.ts

@@ -3,9 +3,26 @@ import {
   homepageSetup,
   returningUserVisitsHomepageWaitForModel,
 } from '../../tests/e2e/utils'
+import {TITLE_NOTES} from '../Notes/component'
 import {TITLE_APPS} from './component'
 
 
+/**
+ * Assert `value` is not null.
+ *
+ * @template T
+ * @param value
+ * @param msg
+ * @return value
+ */
+function assertNotNull<T>(value: T | null, msg: string): T {
+  if (!value) {
+    throw new Error(msg)
+  }
+  return value
+}
+
+
 const {beforeEach, describe} = test
 /**
  * Migrated from cypress/e2e/apps/apps.cy.js
@@ -47,5 +64,58 @@ describe('AppsSideDrawer', () => {
         expect(parseInt(width)).toBeGreaterThan(MIN_FULLSCREEN_WIDTH)
       })
     })
+
+    test('resizing Notes drawer does not push Apps off-screen', async ({page}) => {
+      // Open both Notes and Apps.
+      await page.getByTestId('control-button-notes').click()
+      await page.getByTestId('control-button-apps').click()
+
+      // Sanity: both titles visible.
+      await expect(page.getByTestId(`PanelTitle-${TITLE_NOTES}`)).toBeVisible()
+      await expect(page.getByTestId(`PanelTitle-${TITLE_APPS}`)).toBeVisible()
+
+      const notesDrawer = page.getByTestId('NotesAndPropertiesDrawer')
+      const appsDrawer = page.getByTestId('AppsDrawer')
+
+      const handle = notesDrawer.getByTestId('resize-handle-x')
+      const handleBox = assertNotNull(
+        await handle.boundingBox(),
+        'Expected Notes resize handle to have a bounding box',
+      )
+      const viewerContainer = page.locator('#viewer-container')
+      const viewerBoxBefore = assertNotNull(
+        await viewerContainer.boundingBox(),
+        'Expected viewer container to have a bounding box',
+      )
+
+      // Drag left to widen Notes (this used to push Apps out of view).
+      await page.mouse.move(
+        handleBox.x + (handleBox.width / 2),
+        handleBox.y + (handleBox.height / 2),
+      )
+      await page.mouse.down()
+      const dragDistance = 250
+      await page.mouse.move(handleBox.x - dragDistance, handleBox.y + (handleBox.height / 2))
+      await page.mouse.up()
+
+      // Apps should remain visible and inside the viewport.
+      await expect(page.getByTestId(`PanelTitle-${TITLE_APPS}`)).toBeVisible()
+
+      const appsBox = assertNotNull(
+        await appsDrawer.boundingBox(),
+        'Expected Apps drawer to have a bounding box',
+      )
+      const vw = await page.evaluate(() => window.innerWidth)
+      expect(appsBox.x).toBeGreaterThanOrEqual(0)
+      expect(appsBox.x + (appsBox.width)).toBeLessThanOrEqual(vw + 1)
+
+      // Viewer should not resize when drawers open/resize (drawers overlay the canvas).
+      const viewerBoxAfter = assertNotNull(
+        await viewerContainer.boundingBox(),
+        'Expected viewer container to have a bounding box after resize',
+      )
+      expect(Math.abs(viewerBoxAfter.width - viewerBoxBefore.width)).toBeLessThanOrEqual(1)
+      expect(Math.abs(viewerBoxAfter.width - vw)).toBeLessThanOrEqual(2)
+    })
   })
 })

src/Components/SideDrawer/SideDrawer.jsx

@@ -44,7 +44,9 @@ export default function SideDrawer({
       sx={Object.assign({
         display: isDrawerVisible ? 'flex' : 'none',
         flexDirection: 'row',
-        flexGrow: 1,
+        // Desktop drawers must be fixed-width flex items.
+        // If they grow/shrink, resizing one drawer can push siblings off-screen.
+        ...(isMobile ? {} : {flex: '0 0 auto', flexShrink: 0}),
       }, isMobile ? {
         width: '100%',
         height: drawerHeight,

src/Containers/CadView.jsx

@@ -50,8 +50,6 @@ export default function CadView({
   const hasGitHubIdentity = useStore((state) => state.hasGithubIdentity)
   const customViewSettings = useStore((state) => state.customViewSettings)
   const elementTypesMap = useStore((state) => state.elementTypesMap)
-  const isAppsVisible = useStore((state) => state.isAppsVisible)
-  const isNotesVisible = useStore((state) => state.isNotesVisible)
   const preselectedElementIds = useStore((state) => state.preselectedElementIds)
   const searchIndex = useStore((state) => state.searchIndex)
   const selectedElements = useStore((state) => state.selectedElements)
@@ -68,7 +66,6 @@ export default function CadView({
   const setSelectedElement = useStore((state) => state.setSelectedElement)
   const setSelectedElements = useStore((state) => state.setSelectedElements)
   const setViewer = useStore((state) => state.setViewer)
-  const sidebarWidth = useStore((state) => state.sidebarWidth)
   const viewer = useStore((state) => state.viewer)
 
   // AppSlice
@@ -703,17 +700,8 @@ export default function CadView({
   /* eslint-enable */
 
 
-  // Shrink the scene viewer when drawer is open.  This recenters the
-  // view in the new shrunk canvas, which preserves what the user is
-  // looking at.
-  // TODO(pablo): add render testing
-  useEffect(() => {
-    const isDrawerOpen = isNotesVisible || isAppsVisible
-    if (viewer && !isMobile) {
-      viewer.container.style.width = isDrawerOpen ? `calc(100vw - ${sidebarWidth}px)` : '100vw'
-      viewer.context.resize()
-    }
-  }, [isNotesVisible, isAppsVisible, isMobile, viewer, sidebarWidth])
+  // NOTE: Do not resize the 3D canvas when drawers open/resize.
+  // Drawers should overlay the viewer without changing its dimensions.
 
   useEffect(() => {
     const setViewportHeight = () => {

src/Containers/RootLandscape.jsx

@@ -28,16 +28,17 @@ export default function RootLandscape({pathPrefix, branch, selectWithShiftClickE
   return (
     <Stack
       direction='row'
-      justifyContent='space-between'
-      alignItems='center'
-      sx={{width: '100%', height: isMobile ? `${vh}px` : '100vh'}}
+      justifyContent='flex-start'
+      alignItems='stretch'
+      sx={{width: '100%', height: isMobile ? `${vh}px` : '100vh', overflow: 'hidden'}}
       data-testid='RootLandscape-RootStack'
     >
       {!isMobile &&
        <Box
          sx={{
-           flexBasis: '0%',
-           flexGrow: 1,
+           // Left drawer should take only its own width.
+           flex: '0 0 auto',
+           flexShrink: 0,
          }}
        >
          <NavTreeAndVersionsDrawer
@@ -49,7 +50,7 @@ export default function RootLandscape({pathPrefix, branch, selectWithShiftClickE
       }
       <Stack
         justifyContent='space-between'
-        sx={{width: '100%', height: '100%'}}
+        sx={{flex: '1 1 auto', minWidth: 0, height: '100%'}}
         data-testid='CenterPane'
       >
         <Box sx={{opacity: 0.5}}>
@@ -71,7 +72,7 @@ export default function RootLandscape({pathPrefix, branch, selectWithShiftClickE
           justifyContent='space-between'
           // This pushes bottom bar down
           flexGrow={1}
-          sx={{width: '100%'}}
+          sx={{width: '100%', minWidth: 0}}
           data-testid='RootLandscape-CenterPaneTopStack'
         >
           <ControlsGroup/>

src/Containers/RootLandscape.test.jsx

@@ -0,0 +1,39 @@
+import React from 'react'
+import {render} from '@testing-library/react'
+import ShareMock from '../ShareMock'
+import RootLandscape from './RootLandscape'
+
+
+jest.mock('./ControlsGroup', () => ({
+  __esModule: true,
+  default: () => <div data-testid='MockControlsGroup'/>,
+}))
+
+jest.mock('./OperationsGroup', () => ({
+  __esModule: true,
+  default: () => <div data-testid='MockOperationsGroup'/>,
+}))
+
+describe('RootLandscape', () => {
+  it('center pane is flex and root does not overflow', () => {
+    const {getByTestId} = render(
+      <ShareMock>
+        <RootLandscape
+          pathPrefix=''
+          branch=''
+          selectWithShiftClickEvents={jest.fn()}
+          deselectItems={jest.fn()}
+        />
+      </ShareMock>,
+    )
+
+    const root = getByTestId('RootLandscape-RootStack')
+    const centerPane = getByTestId('CenterPane')
+
+    expect(getComputedStyle(root).overflow).toBe('hidden')
+
+    // Center pane should shrink when drawers grow.
+    expect(getComputedStyle(centerPane).flexGrow).toBe('1')
+    expect(getComputedStyle(centerPane).minWidth).toMatch(/^0(px)?$/)
+  })
+})