Skip to content

Commit

Permalink
Ruler marker resize for non-stretching grid items (#6696)
Browse files Browse the repository at this point in the history
**Problem:**

It should be possible to resize non-stretching grid items via the ruler
markers.

**Fix:**

Because of how grids are rendered, similarly to what happened in
#6691, we need to
calculate the rendered dimensions of grid _cells_ (not their contained
items!) after they are rendered, since for non-stretching grid items
their dimensions (via `getGridChildCellCoordBoundsFromCanvas`) don't
match the ones of the cell(s) they are configured to target.

This PR uses the logic introduced in
#6691 to calculate the
size of a grid cell starting from its item, and uses that to correctly
place the ruler markers for non-stretching items.

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

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Play mode

Fixes #6695
  • Loading branch information
ruggi authored and liady committed Dec 13, 2024
1 parent d36f724 commit 1599d11
Show file tree
Hide file tree
Showing 5 changed files with 222 additions and 159 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
} from '../../../../core/shared/element-template'
import {
localRectangle,
Size,
zeroRectIfNullOrInfinity,
type CanvasRectangle,
type LocalRectangle,
Expand Down Expand Up @@ -753,6 +752,9 @@ export function getGridRelativeContainingBlock(
gridMetadata: ElementInstanceMetadata,
cellMetadata: ElementInstanceMetadata,
cellProperties: GridElementProperties,
options?: {
forcePositionRelative?: boolean
},
): LocalRectangle {
const gridProperties = gridMetadata.specialSizeMeasurements.containerGridProperties

Expand All @@ -779,17 +781,19 @@ export function getGridRelativeContainingBlock(

// Gap needs to be set only if the other two are not present or we'll have rendering issues
// due to how measurements are calculated.
if (
gridMetadata.specialSizeMeasurements.rowGap != null &&
gridMetadata.specialSizeMeasurements.columnGap != null
) {
const gap = gridMetadata.specialSizeMeasurements.gap
gridElement.style.gap = gap == null ? 'initial' : `${gap}px`
} else {
const hasRowGap = gridMetadata.specialSizeMeasurements.rowGap != null
if (hasRowGap) {
const rowGap = gridMetadata.specialSizeMeasurements.rowGap
gridElement.style.rowGap = rowGap == null ? 'initial' : `${rowGap}px`
gridElement.style.rowGap = `${rowGap}px`
}
const hasColumnGap = gridMetadata.specialSizeMeasurements.columnGap != null
if (hasColumnGap) {
const columnGap = gridMetadata.specialSizeMeasurements.columnGap
gridElement.style.columnGap = columnGap == null ? 'initial' : `${columnGap}px`
gridElement.style.columnGap = `${columnGap}px`
}
if (!hasColumnGap && !hasRowGap) {
const gap = gridMetadata.specialSizeMeasurements.gap
gridElement.style.gap = gap == null ? 'initial' : `${gap}px`
}

// Include the padding.
Expand Down Expand Up @@ -855,7 +859,9 @@ export function getGridRelativeContainingBlock(
'row',
)
}
gridChildElement.style.position = cellMetadata.specialSizeMeasurements.position ?? 'initial'
gridChildElement.style.position = options?.forcePositionRelative
? 'relative'
: cellMetadata.specialSizeMeasurements.position ?? 'initial'
// Fill out the entire space available.
gridChildElement.style.top = '0'
gridChildElement.style.left = '0'
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import * as EP from '../../../../core/shared/element-path'
import {
selectComponentsForTest,
setFeatureForBrowserTestsUseInDescribeBlockOnly,
} from '../../../../utils/utils.test-utils'
import { renderTestEditorWithCode } from '../../ui-jsx.test-utils'
import * as EP from '../../../../core/shared/element-path'
import {
RulerMarkerColumnEndTestId,
RulerMarkerRowEndTestId,
RulerMarkerRowStartTestId,
} from '../../controls/grid-controls'
import { mouseDownAtPoint, mouseMoveToPoint, mouseUpAtPoint } from '../../event-helpers.test-utils'
import { renderTestEditorWithCode } from '../../ui-jsx.test-utils'

describe('grid resize element ruler strategy', () => {
setFeatureForBrowserTestsUseInDescribeBlockOnly('Grid Ruler Markers', true)
Expand Down Expand Up @@ -119,6 +119,42 @@ describe('grid resize element ruler strategy', () => {
expect(element.style.gridRowEnd).toBe('3')
}
})

it('can resize a non-stretching pinned element', async () => {
const renderResult = await renderTestEditorWithCode(ProjectCode, 'await-first-dom-report')

await selectComponentsForTest(renderResult, [EP.fromString('sb/grid/pinned-fixed')])

const control = await renderResult.renderedDOM.findByTestId(RulerMarkerColumnEndTestId)
const controlRect = control.getBoundingClientRect()
const startPoint = { x: controlRect.x + 5, y: controlRect.y + 5 }
const endPoint = { x: controlRect.x + 200, y: controlRect.y + 5 }
await mouseDownAtPoint(control, startPoint)
await mouseMoveToPoint(control, endPoint)
await mouseUpAtPoint(control, endPoint)

const element = await renderResult.renderedDOM.findByTestId('pinned-fixed')
expect(element.style.gridColumn).toBe('1 / 3')
expect(element.style.gridRow).toBe('2')
})

it('can resize a non-stretching flow element', async () => {
const renderResult = await renderTestEditorWithCode(ProjectCode, 'await-first-dom-report')

await selectComponentsForTest(renderResult, [EP.fromString('sb/grid/flow-fixed')])

const control = await renderResult.renderedDOM.findByTestId(RulerMarkerColumnEndTestId)
const controlRect = control.getBoundingClientRect()
const startPoint = { x: controlRect.x + 5, y: controlRect.y + 5 }
const endPoint = { x: controlRect.x + 200, y: controlRect.y + 5 }
await mouseDownAtPoint(control, startPoint)
await mouseMoveToPoint(control, endPoint)
await mouseUpAtPoint(control, endPoint)

const element = await renderResult.renderedDOM.findByTestId('flow-fixed')
expect(element.style.gridColumn).toBe('span 2')
expect(element.style.gridRow).toBe('auto')
})
})

const ProjectCode = `
Expand Down Expand Up @@ -161,6 +197,15 @@ export var storyboard = (
data-uid='flow2'
data-testid='flow2'
/>
<div
style={{
backgroundColor: '#f0f',
width: 50,
height: 50,
}}
data-uid='flow-fixed'
data-testid='flow-fixed'
/>
<div
style={{
backgroundColor: '#f09',
Expand All @@ -172,6 +217,17 @@ export var storyboard = (
data-uid='pinned'
data-testid='pinned'
/>
<div
style={{
backgroundColor: '#ff0',
gridColumn: 1,
gridRow: 2,
width: 50,
height: 50,
}}
data-uid='pinned-fixed'
data-testid='pinned-fixed'
/>
</div>
</Storyboard>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { canvasRectangle, isInfinityRectangle } from '../../../../core/shared/ma
import { assertNever } from '../../../../core/shared/utils'
import { gridContainerIdentifier, gridItemIdentifier } from '../../../editor/store/editor-state'
import { isCSSKeyword } from '../../../inspector/common/css-utils'
import { isFillOrStretchModeAppliedOnAnySide } from '../../../inspector/inspector-common'
import {
controlsForGridPlaceholders,
GridResizeControls,
Expand Down Expand Up @@ -91,11 +90,6 @@ export const gridResizeElementRulerStrategy: CanvasStrategyFactory = (
return emptyStrategyApplicationResult
}

if (!isFillOrStretchModeAppliedOnAnySide(canvasState.startingMetadata, selectedElement)) {
// TODO this should be removed to support resizing cells that are not stretching
return emptyStrategyApplicationResult
}

const allCellBounds =
selectedElementMetadata.specialSizeMeasurements.parentGridCellGlobalFrames

Expand Down
Loading

0 comments on commit 1599d11

Please sign in to comment.