Grid cleanup (#6039)

.github/workflows/pull-requests.yml

@@ -468,7 +468,7 @@ jobs:
       - uses: actions/setup-node@v1
         with:
           node-version: 18
-      - uses: pnpm/action-setup@v2
+      - uses: pnpm/action-setup@v4
         name: Install pnpm
         with:
           version: 7.14.2

editor/src/components/canvas/canvas-strategies/canvas-strategy-types.ts

@@ -27,7 +27,8 @@ export interface CustomStrategyState {
 export type GridCustomStrategyState = {
   targetCell: GridCellCoordinates | null
   draggingFromCell: GridCellCoordinates | null
-  rootCell: GridCellCoordinates | null
+  originalRootCell: GridCellCoordinates | null
+  currentRootCell: GridCellCoordinates | null
 }
 
 export type CustomStrategyStatePatch = Partial<CustomStrategyState>
@@ -43,7 +44,8 @@ export function defaultCustomStrategyState(): CustomStrategyState {
     grid: {
       targetCell: null,
       draggingFromCell: null,
-      rootCell: null,
+      originalRootCell: null,
+      currentRootCell: null,
     },
   }
 }

editor/src/components/canvas/canvas-strategies/strategies/grid-helpers.ts

@@ -20,6 +20,7 @@ import type { DragInteractionData } from '../interaction-state'
 import type { GridCustomStrategyState } from '../canvas-strategy-types'
 import type { GridCellCoordinates } from '../../controls/grid-controls'
 import { gridCellCoordinates } from '../../controls/grid-controls'
+import * as EP from '../../../../core/shared/element-path'
 
 export function getGridCellUnderMouse(mousePoint: WindowPoint) {
   return getGridCellAtPoint(mousePoint, false)
@@ -29,14 +30,28 @@ function getGridCellUnderMouseRecursive(mousePoint: WindowPoint) {
   return getGridCellAtPoint(mousePoint, true)
 }
 
+const gridCellTargetIdPrefix = 'grid-cell-target-'
+
+export function gridCellTargetId(
+  gridElementPath: ElementPath,
+  row: number,
+  column: number,
+): string {
+  return gridCellTargetIdPrefix + `${EP.toString(gridElementPath)}-${row}-${column}`
+}
+
+function isGridCellTargetId(id: string): boolean {
+  return id.startsWith(gridCellTargetIdPrefix)
+}
+
 function getGridCellAtPoint(
   windowPoint: WindowPoint,
   duplicating: boolean,
 ): { id: string; coordinates: GridCellCoordinates } | null {
   function maybeRecursivelyFindCellAtPoint(elements: Element[]): Element | null {
     // If this used during duplication, the canvas controls will be in the way and we need to traverse the children too.
     for (const element of elements) {
-      if (element.id.startsWith('gridcell-')) {
+      if (isGridCellTargetId(element.id)) {
         const rect = element.getBoundingClientRect()
         if (rectContainsPoint(windowRectangle(rect), windowPoint)) {
           return element
@@ -85,14 +100,16 @@ export function runGridRearrangeMove(
   commands: CanvasCommand[]
   targetCell: GridCellCoordinates | null
   draggingFromCell: GridCellCoordinates | null
-  rootCell: GridCellCoordinates | null
+  originalRootCell: GridCellCoordinates | null
+  targetRootCell: GridCellCoordinates | null
 } {
   if (interactionData.drag == null) {
     return {
       commands: [],
       targetCell: null,
-      rootCell: null,
+      originalRootCell: null,
       draggingFromCell: null,
+      targetRootCell: null,
     }
   }
 
@@ -108,7 +125,8 @@ export function runGridRearrangeMove(
       commands: [],
       targetCell: null,
       draggingFromCell: null,
-      rootCell: null,
+      originalRootCell: null,
+      targetRootCell: null,
     }
   }
 
@@ -120,8 +138,9 @@ export function runGridRearrangeMove(
     return {
       commands: [],
       targetCell: null,
-      rootCell: null,
+      originalRootCell: null,
       draggingFromCell: null,
+      targetRootCell: null,
     }
   }
 
@@ -131,14 +150,16 @@ export function runGridRearrangeMove(
   // cell of the element, meaning the top-left-most cell the element occupies.
   const draggingFromCell = customState.draggingFromCell ?? newTargetCell
   const rootCell =
-    customState.rootCell ?? gridCellCoordinates(gridProperties.row, gridProperties.column)
+    customState.originalRootCell ?? gridCellCoordinates(gridProperties.row, gridProperties.column)
   const coordsDiff = getCellCoordsDelta(draggingFromCell, rootCell)
 
   // get the new adjusted row
   const row = getCoordBounds(newTargetCell, 'row', gridProperties.width, coordsDiff.row)
   // get the new adjusted column
   const column = getCoordBounds(newTargetCell, 'column', gridProperties.height, coordsDiff.column)
 
+  const targetRootCell = gridCellCoordinates(row.start, column.start)
+
   return {
     commands: [
       setProperty('always', targetElement, create('style', 'gridColumnStart'), column.start),
@@ -147,8 +168,9 @@ export function runGridRearrangeMove(
       setProperty('always', targetElement, create('style', 'gridRowEnd'), row.end),
     ],
     targetCell: newTargetCell,
-    rootCell: rootCell,
+    originalRootCell: rootCell,
     draggingFromCell: draggingFromCell,
+    targetRootCell: targetRootCell,
   }
 }
 

editor/src/components/canvas/canvas-strategies/strategies/grid-rearrange-move-duplicate-strategy.ts

@@ -89,7 +89,8 @@ export const gridRearrangeMoveDuplicateStrategy: CanvasStrategyFactory = (
         commands: moveCommands,
         targetCell: targetGridCell,
         draggingFromCell,
-        rootCell,
+        originalRootCell,
+        targetRootCell,
       } = runGridRearrangeMove(
         targetElement,
         selectedElement,
@@ -117,7 +118,8 @@ export const gridRearrangeMoveDuplicateStrategy: CanvasStrategyFactory = (
           grid: {
             targetCell: targetGridCell,
             draggingFromCell: draggingFromCell,
-            rootCell: rootCell,
+            originalRootCell: originalRootCell,
+            currentRootCell: targetRootCell,
           },
           duplicatedElementNewUids: duplicatedElementNewUids,
         },

editor/src/components/canvas/canvas-strategies/strategies/grid-rearrange-move-strategy.spec.browser2.tsx

@@ -4,6 +4,7 @@ import { selectComponentsForTest } from '../../../../utils/utils.test-utils'
 import { GridCellTestId } from '../../controls/grid-controls'
 import { mouseDragFromPointToPoint } from '../../event-helpers.test-utils'
 import { renderTestEditorWithCode } from '../../ui-jsx.test-utils'
+import { gridCellTargetId } from './grid-helpers'
 
 describe('grid rearrange move strategy', () => {
   it('can rearrange elements on a grid', async () => {
@@ -14,7 +15,9 @@ describe('grid rearrange move strategy', () => {
     await selectComponentsForTest(editor, [elementPathToDrag])
 
     const sourceGridCell = editor.renderedDOM.getByTestId(GridCellTestId(elementPathToDrag))
-    const targetGridCell = editor.renderedDOM.getByTestId('gridcell-0-14')
+    const targetGridCell = editor.renderedDOM.getByTestId(
+      gridCellTargetId(EP.fromString('sb/scene/grid'), 2, 3),
+    )
 
     await mouseDragFromPointToPoint(
       sourceGridCell,

editor/src/components/canvas/canvas-strategies/strategies/grid-rearrange-move-strategy.ts

@@ -73,7 +73,8 @@ export const gridRearrangeMoveStrategy: CanvasStrategyFactory = (
         commands: moveCommands,
         targetCell: targetGridCell,
         draggingFromCell,
-        rootCell,
+        originalRootCell,
+        targetRootCell,
       } = runGridRearrangeMove(
         targetElement,
         selectedElement,
@@ -92,7 +93,8 @@ export const gridRearrangeMoveStrategy: CanvasStrategyFactory = (
         grid: {
           targetCell: targetGridCell,
           draggingFromCell: draggingFromCell,
-          rootCell: rootCell,
+          originalRootCell: originalRootCell,
+          currentRootCell: targetRootCell,
         },
       })
     },

editor/src/components/canvas/controls/grid-controls.tsx

@@ -46,7 +46,7 @@ import {
 import { windowToCanvasCoordinates } from '../dom-lookup'
 import { CanvasOffsetWrapper } from './canvas-offset-wrapper'
 import { useColorTheme } from '../../../uuiui'
-import { getGridCellUnderMouse } from '../canvas-strategies/strategies/grid-helpers'
+import { gridCellTargetId } from '../canvas-strategies/strategies/grid-helpers'
 
 export const GridCellTestId = (elementPath: ElementPath) => `grid-cell-${EP.toString(elementPath)}`
 
@@ -56,6 +56,10 @@ export function gridCellCoordinates(row: number, column: number): GridCellCoordi
   return { row: row, column: column }
 }
 
+export function gridCellCoordinatesToString(coords: GridCellCoordinates): string {
+  return `${coords.row}:${coords.column}`
+}
+
 function getCellsCount(template: GridAutoOrTemplateBase | null): number {
   if (template == null) {
     return 0
@@ -122,10 +126,18 @@ export const GridControls = controlForStrategyMemoized(() => {
     'GridControls activelyDraggingOrResizingCell',
   )
 
-  const { hoveringCell, hoveringStart, mouseCanvasPosition } = useMouseMove(
-    activelyDraggingOrResizingCell,
+  const { hoveringStart, mouseCanvasPosition } = useMouseMove(activelyDraggingOrResizingCell)
+
+  const targetRootCell = useEditorState(
+    Substores.restOfStore,
+    (store) => store.strategyState.customStrategyState.grid.currentRootCell,
+    'GridControls targetRootCell',
+  )
+  const targetRootCellId = React.useMemo(
+    () => (targetRootCell == null ? null : gridCellCoordinatesToString(targetRootCell)),
+    [targetRootCell],
   )
-  useSnapAnimation(hoveringCell, controls)
+  useSnapAnimation(targetRootCellId, controls)
 
   const dragging = useEditorState(
     Substores.canvas,
@@ -375,12 +387,12 @@ export const GridControls = controlForStrategyMemoized(() => {
     <React.Fragment>
       <CanvasOffsetWrapper>
         {/* grid lines */}
-        {grids.map((grid, index) => {
+        {grids.map((grid) => {
           const placeholders = Array.from(Array(grid.cells).keys())
 
           return (
             <div
-              key={`grid-${index}`}
+              key={`grid-${EP.toString(grid.elementPath)}`}
               style={{
                 position: 'absolute',
                 top: grid.frame.y,
@@ -404,10 +416,10 @@ export const GridControls = controlForStrategyMemoized(() => {
                     : `${grid.padding.top}px ${grid.padding.right}px ${grid.padding.bottom}px ${grid.padding.left}px`,
               }}
             >
-              {placeholders.map((cell, cellIndex) => {
-                const countedRow = Math.floor(cellIndex / grid.columns) + 1
-                const countedColumn = Math.floor(cellIndex % grid.columns) + 1
-                const id = `gridcell-${index}-${cell}`
+              {placeholders.map((cell) => {
+                const countedRow = Math.floor(cell / grid.columns) + 1
+                const countedColumn = Math.floor(cell % grid.columns) + 1
+                const id = gridCellTargetId(grid.elementPath, countedRow, countedColumn)
                 const dotgridColor =
                   activelyDraggingOrResizingCell != null
                     ? features.Grid.dotgridColor
@@ -506,30 +518,6 @@ export const GridControls = controlForStrategyMemoized(() => {
               })}
               key={GridCellTestId(cell.elementPath)}
               data-testid={GridCellTestId(cell.elementPath)}
-              style={{
-                position: 'absolute',
-                top: cell.globalFrame.y,
-                left: cell.globalFrame.x,
-                width: cell.globalFrame.width,
-                height: cell.globalFrame.height,
-                display: 'flex',
-                justifyContent: 'flex-end',
-                alignItems: 'flex-end',
-              }}
-            />
-          )
-        })}
-        {/* cell targets */}
-        {cells.map((cell) => {
-          return (
-            <div
-              onMouseDown={startInteractionWithUid({
-                uid: EP.toUid(cell.elementPath),
-                frame: cell.globalFrame,
-                row: cell.row,
-                column: cell.column,
-              })}
-              key={`grid-cell-${EP.toString(cell.elementPath)}`}
               style={{
                 position: 'absolute',
                 top: cell.globalFrame.y,
@@ -620,9 +608,12 @@ export const GridControls = controlForStrategyMemoized(() => {
                     event.preventDefault()
                   }
 
+                  const id = `grid-column-handle-${dimensionIndex}`
+
                   return (
                     <div
-                      data-testid={`grid-column-handle-${dimensionIndex}`}
+                      key={id}
+                      data-testid={id}
                       style={{
                         position: 'absolute',
                         left: workingPrefix - 15,
@@ -687,9 +678,13 @@ export const GridControls = controlForStrategyMemoized(() => {
                     event.stopPropagation()
                     event.preventDefault()
                   }
+
+                  const id = `grid-row-handle-${dimensionIndex}`
+
                   return (
                     <div
-                      data-testid={`grid-row-handle-${dimensionIndex}`}
+                      key={id}
+                      data-testid={id}
                       style={{
                         position: 'absolute',
                         left: grid.frame.x - 50,
@@ -722,20 +717,18 @@ export const GridControls = controlForStrategyMemoized(() => {
   )
 })
 
-function useSnapAnimation(hoveringCell: string | null, controls: AnimationControls) {
+function useSnapAnimation(targetRootCellId: string | null, controls: AnimationControls) {
   const features = useRollYourOwnFeatures()
   React.useEffect(() => {
-    if (!features.Grid.animateSnap || hoveringCell == null) {
+    if (!features.Grid.animateSnap || targetRootCellId == null) {
       return
     }
     void controls.start(SHADOW_SNAP_ANIMATION)
-  }, [hoveringCell, controls, features.Grid.animateSnap])
+  }, [targetRootCellId, controls, features.Grid.animateSnap])
 }
 
 function useMouseMove(activelyDraggingOrResizingCell: string | null) {
-  const [hoveringCell, setHoveringCell] = React.useState<string | null>(null)
   const [hoveringStart, setHoveringStart] = React.useState<{
-    id: string
     point: CanvasPoint
   } | null>(null)
   const [mouseCanvasPosition, setMouseCanvasPosition] = React.useState<CanvasPoint>(
@@ -761,33 +754,27 @@ function useMouseMove(activelyDraggingOrResizingCell: string | null) {
         return
       }
 
-      // TODO most of this logic can be simplified consistently
-      // by moving the hovering cell info to the editor state, dispatched
-      // from the grid-rearrange-move-strategy logic.
-      const cellUnderMouse = getGridCellUnderMouse(windowPoint({ x: e.clientX, y: e.clientY }))
-      if (cellUnderMouse != null) {
-        setHoveringCell(cellUnderMouse.id)
-
-        const newMouseCanvasPosition = windowToCanvasCoordinates(
-          canvasScale,
-          canvasOffset,
-          windowPoint({ x: e.clientX, y: e.clientY }),
-        ).canvasPositionRaw
-        setMouseCanvasPosition(newMouseCanvasPosition)
-
-        setHoveringStart((start) => {
-          if (start == null || start.id !== cellUnderMouse.id) {
-            return { id: cellUnderMouse.id, point: canvasPoint(newMouseCanvasPosition) }
+      const newMouseCanvasPosition = windowToCanvasCoordinates(
+        canvasScale,
+        canvasOffset,
+        windowPoint({ x: e.clientX, y: e.clientY }),
+      ).canvasPositionRaw
+      setMouseCanvasPosition(newMouseCanvasPosition)
+
+      setHoveringStart((start) => {
+        if (start == null) {
+          return {
+            point: canvasPoint(newMouseCanvasPosition),
           }
-          return start
-        })
-      }
+        }
+        return start
+      })
     }
     window.addEventListener('mousemove', handleMouseMove)
     return function () {
       window.removeEventListener('mousemove', handleMouseMove)
     }
   }, [activelyDraggingOrResizingCell, canvasOffset, canvasScale])
 
-  return { hoveringCell, hoveringStart, mouseCanvasPosition }
+  return { hoveringStart, mouseCanvasPosition }
 }