Skip to content

Commit

Permalink
No dom query in grid element resize strategy (#6373)
Browse files Browse the repository at this point in the history
**Problem:**
Another step to remove dom api dependency from grid strategies: the
grid-resize-element strategy uses the new `getGlobalFramesOfGridCells`
to detect the grid cell under the mouse.

**Commit Details:** 

- Fixed `getGlobalFramesOfGridCells` problem: it calculated the local
frame not the global frame....
- Fixed and improved `getGlobalFramesOfGridCells` test to include
differend padding/row-padding/column-padding values
- Memoized `getGlobalFramesOfGridCells`: this is a quite fast function
so it wouldn't be important to memoize it, but in most cases we run it
for the same grid cell (because we are doing an interaction in it), so
it still makes sense to memoize. I just uses `memoize` with the default
config (which allows memoization of 5 values)
- Created a new `getGridCellUnderMouse` version called
`getGridCellUnderMouseFromMetadata`, and call that from the strategy
- I modified the strategy to use the canvas and not the window
coordinate system. Unfortunately,
`GridCustomStrategyState.targetCellData` uses the window coordinate
system, and that is used in multiple strategies, so I postponed to
change that too. I introduced a new type `TargetGridCellDataCanvas`
(similar to `TargetGridCellData`), I plan to migrate to this new type
everywhere.

**TODO:**

The strategy is still not the best. A downside of querying the grid
cells under the mouse is that the strategy is broken when the mouse is
not over the grid (even though the resize rectangle follows the mouse),
see https://screenshot.click/16-30-s1avn-hgu83.mp4

Using the metadata it is possible to implement a much better strategy
which can calculate where to resize to even when the mouse is not over
the grid. That can come in a subsequent PR.

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Preview mode
  • Loading branch information
gbalint authored Sep 17, 2024
1 parent 043570d commit b1d8588
Show file tree
Hide file tree
Showing 5 changed files with 173 additions and 111 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ import {
} from '../../../../core/shared/math-utils'
import { canvasPointToWindowPoint } from '../../dom-lookup'
import * as EP from '../../../../core/shared/element-path'
import {
getGlobalFramesOfGridCells,
type GridCellGlobalFrames,
type TargetGridCellDataCanvas,
} from './grid-helpers'
import type { CanvasPoint } from '../../../../core/shared/math-utils'

export type GridCellCoordinates = { row: number; column: number }

Expand All @@ -27,6 +33,38 @@ export function gridCellTargetId(
return gridCellTargetIdPrefix + `${EP.toString(gridElementPath)}-${row}-${column}`
}

export function getGridCellUnderMouseFromMetadata(
grid: ElementInstanceMetadata,
canvasPoint: CanvasPoint,
): TargetGridCellDataCanvas | null {
const gridCellGlobalFrames = getGlobalFramesOfGridCells(grid)

if (gridCellGlobalFrames == null) {
return null
}

return getGridCellUnderPoint(gridCellGlobalFrames, canvasPoint)
}

// TODO: we could optimize this with binary search, but huge grids are rare
function getGridCellUnderPoint(
gridCellGlobalFrames: GridCellGlobalFrames,
canvasPoint: CanvasPoint,
): TargetGridCellDataCanvas | null {
for (let i = 0; i < gridCellGlobalFrames.length; i++) {
for (let j = 0; j < gridCellGlobalFrames[i].length; j++) {
if (rectContainsPoint(gridCellGlobalFrames[i][j], canvasPoint)) {
return {
gridCellCoordinates: gridCellCoordinates(i + 1, j + 1),
cellCanvasRectangle: gridCellGlobalFrames[i][j],
}
}
}
}
return null
}

// TODO: should be superseded by getGridCellUnderMouseFromMetadata
export function getGridCellUnderMouse(mousePoint: WindowPoint) {
return getGridCellAtPoint(mousePoint, false)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { matchInlineSnapshotBrowser } from '../../../../../test/karma-snapshots'
import { runDOMWalker } from '../../../editor/actions/action-creators'
import { makeTestProjectCodeWithSnippet, renderTestEditorWithCode } from '../../ui-jsx.test-utils'
import { getGlobalFramesOfGridCellsFromMetadata } from './grid-helpers'
import { getGlobalFramesOfGridCells } from './grid-helpers'

describe('Grids', () => {
it('can calculate global frames of grid cells', async () => {
Expand All @@ -17,8 +17,9 @@ describe('Grids', () => {
display: 'grid',
gridTemplateColumns: '150px min-content 1fr 1fr',
gridTemplateRows: 'min-content 1fr 1fr 1fr 1fr',
gridGap: 10,
padding: 10,
rowGap: 10,
columnGap: 20,
padding: 5,
}}
data-uid={'grid'}
>
Expand Down Expand Up @@ -110,7 +111,7 @@ describe('Grids', () => {

// non-grids don't have cell measurements:
expect(
getGlobalFramesOfGridCellsFromMetadata(
getGlobalFramesOfGridCells(
editor.getEditorState().editor.jsxMetadata[
'utopia-storyboard-uid/scene-aaa/app-entity:grid/child'
],
Expand All @@ -119,7 +120,7 @@ describe('Grids', () => {

// grids have cell measurements:
matchInlineSnapshotBrowser(
getGlobalFramesOfGridCellsFromMetadata(
getGlobalFramesOfGridCells(
editor.getEditorState().editor.jsxMetadata[
'utopia-storyboard-uid/scene-aaa/app-entity:grid'
],
Expand All @@ -129,130 +130,130 @@ describe('Grids', () => {
Object {
\"height\": 50,
\"width\": 150,
\"x\": 10,
\"y\": 10,
\"x\": 132,
\"y\": 89,
},
Object {
\"height\": 50,
\"width\": 80,
\"x\": 170,
\"y\": 10,
\"x\": 302,
\"y\": 89,
},
Object {
\"height\": 50,
\"width\": 119.5,
\"x\": 260,
\"y\": 10,
\"width\": 109.5,
\"x\": 402,
\"y\": 89,
},
Object {
\"height\": 50,
\"width\": 119.5,
\"x\": 389.5,
\"y\": 10,
\"width\": 109.5,
\"x\": 531.5,
\"y\": 89,
},
],
Array [
Object {
\"height\": 83.25,
\"height\": 85.75,
\"width\": 150,
\"x\": 10,
\"y\": 70,
\"x\": 132,
\"y\": 149,
},
Object {
\"height\": 83.25,
\"height\": 85.75,
\"width\": 80,
\"x\": 170,
\"y\": 70,
\"x\": 302,
\"y\": 149,
},
Object {
\"height\": 83.25,
\"width\": 119.5,
\"x\": 260,
\"y\": 70,
\"height\": 85.75,
\"width\": 109.5,
\"x\": 402,
\"y\": 149,
},
Object {
\"height\": 83.25,
\"width\": 119.5,
\"x\": 389.5,
\"y\": 70,
\"height\": 85.75,
\"width\": 109.5,
\"x\": 531.5,
\"y\": 149,
},
],
Array [
Object {
\"height\": 83.25,
\"height\": 85.75,
\"width\": 150,
\"x\": 10,
\"y\": 163.25,
\"x\": 132,
\"y\": 244.75,
},
Object {
\"height\": 83.25,
\"height\": 85.75,
\"width\": 80,
\"x\": 170,
\"y\": 163.25,
\"x\": 302,
\"y\": 244.75,
},
Object {
\"height\": 83.25,
\"width\": 119.5,
\"x\": 260,
\"y\": 163.25,
\"height\": 85.75,
\"width\": 109.5,
\"x\": 402,
\"y\": 244.75,
},
Object {
\"height\": 83.25,
\"width\": 119.5,
\"x\": 389.5,
\"y\": 163.25,
\"height\": 85.75,
\"width\": 109.5,
\"x\": 531.5,
\"y\": 244.75,
},
],
Array [
Object {
\"height\": 83.25,
\"height\": 85.75,
\"width\": 150,
\"x\": 10,
\"y\": 256.5,
\"x\": 132,
\"y\": 340.5,
},
Object {
\"height\": 83.25,
\"height\": 85.75,
\"width\": 80,
\"x\": 170,
\"y\": 256.5,
\"x\": 302,
\"y\": 340.5,
},
Object {
\"height\": 83.25,
\"width\": 119.5,
\"x\": 260,
\"y\": 256.5,
\"height\": 85.75,
\"width\": 109.5,
\"x\": 402,
\"y\": 340.5,
},
Object {
\"height\": 83.25,
\"width\": 119.5,
\"x\": 389.5,
\"y\": 256.5,
\"height\": 85.75,
\"width\": 109.5,
\"x\": 531.5,
\"y\": 340.5,
},
],
Array [
Object {
\"height\": 83.25,
\"height\": 85.75,
\"width\": 150,
\"x\": 10,
\"y\": 349.75,
\"x\": 132,
\"y\": 436.25,
},
Object {
\"height\": 83.25,
\"height\": 85.75,
\"width\": 80,
\"x\": 170,
\"y\": 349.75,
\"x\": 302,
\"y\": 436.25,
},
Object {
\"height\": 83.25,
\"width\": 119.5,
\"x\": 260,
\"y\": 349.75,
\"height\": 85.75,
\"width\": 109.5,
\"x\": 402,
\"y\": 436.25,
},
Object {
\"height\": 83.25,
\"width\": 119.5,
\"x\": 389.5,
\"y\": 349.75,
\"height\": 85.75,
\"width\": 109.5,
\"x\": 531.5,
\"y\": 436.25,
},
],
]`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import {
gridCellCoordinates,
} from './grid-cell-bounds'
import type { Sides } from 'utopia-api/core'
import { memoize } from '../../../../core/shared/memoize'

export function runGridRearrangeMove(
targetElement: ElementPath,
Expand Down Expand Up @@ -350,6 +351,11 @@ export interface TargetGridCellData {
cellWindowRectangle: WindowRectangle
}

export interface TargetGridCellDataCanvas {
gridCellCoordinates: GridCellCoordinates
cellCanvasRectangle: CanvasRectangle
}

export function getTargetCell(
previousTargetCell: GridCellCoordinates | null,
duplicating: boolean,
Expand Down Expand Up @@ -612,23 +618,18 @@ function getGridPositionIndex(props: {
return (props.row - 1) * props.gridTemplateColumns + props.column - 1
}

export function getGlobalFramesOfGridCellsFromMetadata(
export type GridCellGlobalFrames = Array<Array<CanvasRectangle>>

function getGlobalFramesOfGridCellsInner(
metadata: ElementInstanceMetadata,
): Array<Array<CanvasRectangle>> | null {
return getGlobalFramesOfGridCells(metadata.specialSizeMeasurements)
}
): GridCellGlobalFrames | null {
const { globalFrame } = metadata
if (globalFrame == null || isInfinityRectangle(globalFrame)) {
return null
}

const { containerGridProperties, padding, rowGap, columnGap } = metadata.specialSizeMeasurements

export function getGlobalFramesOfGridCells({
containerGridProperties,
rowGap,
columnGap,
padding,
}: {
containerGridProperties: GridContainerProperties
rowGap: number | null
columnGap: number | null
padding: Sides
}): Array<Array<CanvasRectangle>> | null {
const columnWidths = gridTemplateToNumbers(containerGridProperties.gridTemplateColumns)

const rowHeights = gridTemplateToNumbers(containerGridProperties.gridTemplateRows)
Expand All @@ -638,9 +639,9 @@ export function getGlobalFramesOfGridCells({
}

const cellRects: Array<Array<CanvasRectangle>> = []
let yOffset = padding.top ?? 0
let yOffset = globalFrame.y + (padding.top ?? 0)
rowHeights.forEach((height) => {
let xOffset = padding.left ?? 0
let xOffset = globalFrame.x + (padding.left ?? 0)
const rowRects: CanvasRectangle[] = []
columnWidths.forEach((width) => {
const rect = canvasRectangle({ x: xOffset, y: yOffset, width: width, height: height })
Expand All @@ -654,6 +655,8 @@ export function getGlobalFramesOfGridCells({
return cellRects
}

export const getGlobalFramesOfGridCells = memoize(getGlobalFramesOfGridCellsInner)

function gridTemplateToNumbers(gridTemplate: GridTemplate | null): Array<number> | null {
if (gridTemplate?.type !== 'DIMENSIONS') {
return null
Expand Down
Loading

0 comments on commit b1d8588

Please sign in to comment.