Preserve span when changing grid item position #6633

ruggi · 2024-11-13T12:03:53Z

Problem:

span values for grid child positioning props are wiped when using gridChangeElementLocationStrategy.

Fix:

Rework the way grid element props are calculated when changing element location pins, so that spans are respected and kept in place if present.

The behavior is now so that:

when it makes sense to do so, return shorthand versions of grid positioning props
when moving an element with span values, support moving them when the span is in the starting prop as well as the ending prop

Examples for moving a item one cell to the right:

Initial config	Result
`gridColumn: 'span 2'`	`gridColumn: 'span 2 / 4'`
`gridColumn: '2 / span 2'`	`gridColumn: '3 / span 2'`
`gridColumnStart: 'span 2'`	`gridColumn: 'span 2 / 4'`
`gridColumnEnd: 'span 2'`	`gridColumn: '3 / span 2'`

Note: a subsequent PR will take care of also keepign span intact for the reorder strategy.

Fixes #6639

github-actions · 2024-11-13T12:06:46Z

Try me

relativeci · 2024-11-13T12:09:46Z

#15130 Bundle Size — 58.08MiB (~+0.01%).

1485b74(current) vs d1d46c1 master#15127(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics

2 changes

1 regression

	Current #15130	Baseline #15127
Initial JS	`41.05MiB`(`~+0.01%`)	`41.05MiB`
Initial CSS	`0B`	`0B`
Cache Invalidation	`18.66%`	`18.01%`
Chunks	`20`	`20`
Assets	`22`	`22`
Modules	`4169`	`4169`
Duplicate Modules	`213`	`213`
Duplicate Code	`27.3%`	`27.3%`
Packages	`477`	`477`
Duplicate Packages	`70`	`70`

Bundle size by type

2 changes

1 regression

1 improvement

	Current #15130	Baseline #15127
JS	`58.07MiB` (`~+0.01%`)	`58.07MiB`
HTML	`7.37KiB` (`-0.25%`)	`7.39KiB`

Bundle analysis report Branch feat/preserve-span-change-positi... Project dashboard

^{Generated by RelativeCI Documentation Report issue}

ruggi · 2024-11-13T18:06:57Z

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

+  ): {
+    property:
+      | 'gridColumn'
+      | 'gridColumnStart'


this and the other start are currently unused but I figured it's just easy to leave them here if we want to play around with this logic a bit

ruggi · 2024-11-13T18:08:08Z

.../canvas/canvas-strategies/strategies/grid-element-change-location-strategy.spec.browser2.tsx

@@ -594,9 +699,9 @@ export var storyboard = (
            child.style
          expect({ top, left, gridColumnStart, gridColumnEnd, gridRowStart, gridRowEnd }).toEqual({
            gridColumnStart: '1',
-            gridColumnEnd: '',
+            gridColumnEnd: 'auto',


reminder: these are NOT being serialized, it's just that the computed style defaults these to auto if a shorthand is used (which is fine)

# Conflicts: # editor/src/components/canvas/canvas-strategies/strategies/grid-change-element-location-strategy.ts

balazsbajorics · 2024-11-14T09:50:50Z

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

+export function isAutoGridPin(v: GridPositionOrSpan): boolean {
+  return isCSSKeyword(v) && v.value === 'auto'
+}
+
 export function setGridPropsCommands(


these command generator functions are usually called get * commands or create * commands. And since this is not setting all grid props, just the placement are props, I would rename it to something like getCommandsForGritItemPlacement

renamed in b75261c (#6633)

balazsbajorics · 2024-11-14T10:04:49Z

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

+      (isCSSKeyword(endPosition) && endPosition.value === 'auto') ||
+      (isGridPositionNumericValue(endPosition) &&
+        isGridPositionNumericValue(startPosition) &&
+        (endPosition.numericalPosition ?? 0) >= (startPosition.numericalPosition ?? 0) &&
+        (endPosition.numericalPosition ?? 0) - (startPosition.numericalPosition ?? 0) <= 1) ||
+      startValue === endValue


could you move this to a helper function with names? I had to ask Cursor what this code does

(btw here's what it wrote

Is the end position 'auto'? Are both positions numeric, and is the end position either equal to or one greater than the start position? Are the start and end values equal?

)

// ... existing code ... function shouldUseSimplifiedGridFormat( startPosition: GridPositionOrSpan, endPosition: GridPositionOrSpan, startValue: string | number, endValue: string | number, ): boolean { // Case 1: End position is 'auto' if (isCSSKeyword(endPosition) && endPosition.value === 'auto') { return true } // Case 2: Both positions are numeric and span only 1 cell if (isGridPositionNumericValue(startPosition) && isGridPositionNumericValue(endPosition)) { const start = startPosition.numericalPosition ?? 0 const end = endPosition.numericalPosition ?? 0 const isOneOrNoCellSpan = end - start <= 1 && end >= start if (isOneOrNoCellSpan) { return true } } // Case 3: Start and end values are the same return startValue === endValue } // Then in the original code: if (shouldUseSimplifiedGridFormat(startPosition, endPosition, startValue, endValue)) { return { property: axis === 'column' ? 'gridColumn' : 'gridRow', value: startValue, } } // ... existing code ...

impressive suggestion from Cursor!

done in fc142ff (#6633)

balazsbajorics · 2024-11-14T10:18:20Z

.../src/components/canvas/canvas-strategies/strategies/grid-change-element-location-strategy.ts

-    (elementProperties) => {
-      function shouldKeep(shorthandProp: 'gridColumn' | 'gridRow'): boolean {
-        const longhandProp = shorthandProp === 'gridColumn' ? 'gridColumnEnd' : 'gridRowEnd'
+  function getUpdatedPins(


please rewrite this to not use mutation!

done in 1485b74 (#6633)

balazsbajorics

I left some feedback about coding style, but overall looks good!

**Problem:** `span` values for grid child positioning props are wiped when using `gridChangeElementLocationStrategy`. **Fix:** Rework the way grid element props are calculated when changing element location pins, so that `spans` are respected and kept in place if present. The behavior is now so that: - when it makes sense to do so, return shorthand versions of grid positioning props - when moving an element with `span` values, support moving them when the span is in the starting prop as well as the ending prop Examples for moving a item one cell to the right: | Initial config | Result | |------------|------------| | `gridColumn: 'span 2'` | `gridColumn: 'span 2 / 4'` | | `gridColumn: '2 / span 2'` | `gridColumn: '3 / span 2'` | | `gridColumnStart: 'span 2'` | `gridColumn: 'span 2 / 4'` | | `gridColumnEnd: 'span 2'` | `gridColumn: '3 / span 2'` | **Note:** a subsequent PR will take care of also keepign `span` intact for the reorder strategy. Fixes #6639

preserve span when changing position

8b2e773

ruggi added 4 commits November 13, 2024 16:16

update for tests

7c9c00c

adjust tests

ff13197

update tests

a7c4a51

span tests

02e90ef

ruggi commented Nov 13, 2024

View reviewed changes

ruggi marked this pull request as ready for review November 13, 2024 18:19

ruggi changed the title ~~Preserve span when changing position~~ Preserve span when changing grid item position Nov 13, 2024

Merge branch 'master' into feat/preserve-span-change-position

ea53382

# Conflicts: # editor/src/components/canvas/canvas-strategies/strategies/grid-change-element-location-strategy.ts

balazsbajorics reviewed Nov 14, 2024

View reviewed changes

rename to getCommandsForGridItemPlacement

b75261c

balazsbajorics reviewed Nov 14, 2024

View reviewed changes

ruggi added 2 commits November 14, 2024 11:11

helper for clarity

fc142ff

whitespace

84f349c

balazsbajorics reviewed Nov 14, 2024

View reviewed changes

balazsbajorics approved these changes Nov 14, 2024

View reviewed changes

early returns

1485b74

gbalint approved these changes Nov 14, 2024

View reviewed changes

ruggi merged commit 6a652a3 into master Nov 14, 2024
16 checks passed

ruggi deleted the feat/preserve-span-change-position branch November 14, 2024 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve span when changing grid item position #6633

Preserve span when changing grid item position #6633

ruggi commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading

relativeci bot commented Nov 13, 2024 •

edited

Loading

ruggi Nov 13, 2024

ruggi Nov 13, 2024

balazsbajorics Nov 14, 2024

ruggi Nov 14, 2024

balazsbajorics Nov 14, 2024

balazsbajorics Nov 14, 2024

ruggi Nov 14, 2024

balazsbajorics Nov 14, 2024

ruggi Nov 14, 2024

balazsbajorics left a comment

Preserve span when changing grid item position #6633

Preserve span when changing grid item position #6633

Conversation

ruggi commented Nov 13, 2024 • edited Loading

github-actions bot commented Nov 13, 2024 • edited Loading

relativeci bot commented Nov 13, 2024 • edited Loading

#15130 Bundle Size — 58.08MiB (~+0.01%).

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balazsbajorics left a comment

Choose a reason for hiding this comment

ruggi commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading

relativeci bot commented Nov 13, 2024 •

edited

Loading