Split grid move strategies #6606
Conversation
#15036 Bundle Size — 58.05MiB (~+0.01%).945b36f(current) vs d1d46c1 master#15035(baseline) Warning Bundle contains 70 duplicate packages – View duplicate packages Bundle metrics
|
| Current #15036 |
Baseline #15035 |
|
|---|---|---|
 Initial JS |
41.03MiB(+0.01%) |
41.02MiB |
 Initial CSS |
0B |
0B |
 Cache Invalidation |
18.08% |
18.25% |
| Chunks |
20 |
20 |
| Assets |
22 |
22 |
| Modules |
4166(+0.02%) |
4165 |
| Duplicate Modules |
213 |
213 |
| Duplicate Code |
27.31% |
27.31% |
| Packages |
477 |
477 |
| Duplicate Packages |
70 |
70 |
Bundle size by type 2 changes 1 regression 
1 improvement
| Current #15036 |
Baseline #15035 |
|
|---|---|---|
| JS |
58.04MiB (~+0.01%) |
58.04MiB |
| HTML |
7.37KiB (-0.25%) |
7.39KiB |
Bundle analysis report Branch chore/split-grid-move-strats Project dashboard
Generated by RelativeCI Documentation Report issue
editor/src/components/canvas/canvas-strategies/strategies/grid-helpers.ts
Outdated
Show resolved
Hide resolved
| @@ -273,7 +273,7 @@ function gridReparentCommands( | |||
|
|
|||
| const gridPropsCommands = | |||
| gridCellGlobalFrames != null && gridTemplate != null | |||
| ? runGridRearrangeMove( | |||
| ? runGridMoveRearrange( | |||
There was a problem hiding this comment.
This is probably more like a a followup PR problem, but I think this is a regression, this way reparent to grid always uses rearrange, so there is no way to just drop an element into a grid without it getting pinned down (which was possible before this PR).
There was a problem hiding this comment.
indeed, but imo this is something to be done with a followup PR, because we need to decide how to tackle this interaction between blending strategies
editor/src/components/canvas/canvas-strategies/strategies/grid-helpers.ts
Outdated
Show resolved
Hide resolved
editor/src/components/canvas/canvas-strategies/strategies/grid-helpers.ts
Outdated
Show resolved
Hide resolved
editor/src/components/canvas/canvas-strategies/strategies/grid-helpers.ts
Outdated
Show resolved
Hide resolved
editor/src/components/canvas/canvas-strategies/strategies/grid-helpers.ts
Outdated
Show resolved
Hide resolved
editor/src/components/canvas/canvas-strategies/strategies/grid-move-reorder-strategy.ts
Outdated
Show resolved
Hide resolved
| } | ||
| const { mouseCellPosInOriginalElement } = gridConfig | ||
|
|
||
| const gridPath = EP.parentPath(pathForCommands) |
There was a problem hiding this comment.
not related to this PR, but please leave a comment here that this needs to be deleted – we do not know if EP.parentPath points to a grid or not, nor should we know that. we are tracking this work as part of the components suppport in the paper doc
There was a problem hiding this comment.
done, I marked all usages with comments
| default: | ||
| assertNever(moveType) | ||
| } | ||
| } | ||
|
|
||
| export function runGridMoveReorder( |
There was a problem hiding this comment.
this is just reorder, right? no need to have move in the name anymore
There was a problem hiding this comment.
I agree but for consistency I kept the naming prefixes, imho naming must be fixed (e.g. "rearrange") but incrementally
| @@ -155,6 +130,7 @@ export function runGridRearrangeMove( | |||
| gridPath, | |||
| targetCellData?.gridCellCoordinates ?? null, | |||
| gridCellCoordinates(row, column), | |||
| 'valid', | |||
| ) | |||
|
|
|||
| switch (moveType) { | |||
There was a problem hiding this comment.
it feels wrong that runGridMoveRearrange has to deal with absolute move – in a follow-up PR, we should run the real absolute strategy and remove absolute-related code from here.
There was a problem hiding this comment.
absolute has been taken out into its own strategy
...omponents/canvas/canvas-strategies/strategies/grid-rearrange-move-strategy.spec.browser2.tsx
Outdated
Show resolved
Hide resolved
editor/src/components/canvas/commands/show-grid-controls-command.ts
Outdated
Show resolved
Hide resolved
editor/src/components/canvas/canvas-strategies/strategies/resize-grid-strategy.ts
Show resolved
Hide resolved
...omponents/canvas/canvas-strategies/strategies/rearrange-grid-swap-strategy.spec.browser2.tsx
Show resolved
Hide resolved
**Problem:** The grid "move" strategies should be renamed to better reflect what they do and to be less confusing. **Fix:** This PR is a followup to #6606 1. Rename "move rearrange" to "change grid element location" (and duplicate) 2. Rename "move reorder" to "reorder" Fixes #6620 --------- Co-authored-by: Federico Ruggi <federico.ruggi@shopify.com>
**Problem:** The move strategies for grids should be split both functionally as well as implementation-wise. **Fix:** This PR does a bunch of things in order to clean up the grid move strategies: - Remove the swap strategy - Split the rearrange strategy into two distinct strategies: rearrange and reorder - Take out the absolute move portions out of the rearrange strategy into their own strategy - The reorder strategy performs in-flow moves - The rearrange strategy performs pinned moves - When the reorder strategy is active, don't show the animation as well as the target cell outline - For pinned elements, give precedence to the rearrange strategy - For in-flow elements, give precedence to the reorder strategy - Marked all usages of `EP.parentPath` to detect the parent grid as `TODO` for a future PR fixing that in favor of actual grid metadata **Note:** Imho the tests should be also split in two, in this PR I only updated the existing ones (which are still relevant tho!) but it'd be good to have an incremental PR to do that split as well. Fixes #6607
**Problem:** The grid "move" strategies should be renamed to better reflect what they do and to be less confusing. **Fix:** This PR is a followup to #6606 1. Rename "move rearrange" to "change grid element location" (and duplicate) 2. Rename "move reorder" to "reorder" Fixes #6620 --------- Co-authored-by: Federico Ruggi <federico.ruggi@shopify.com>
Problem:
The move strategies for grids should be split both functionally as well as implementation-wise.
Fix:
This PR does a bunch of things in order to clean up the grid move strategies:
EP.parentPathto detect the parent grid asTODOfor a future PR fixing that in favor of actual grid metadataNote:
Imho the tests should be also split in two, in this PR I only updated the existing ones (which are still relevant tho!) but it'd be good to have an incremental PR to do that split as well.
Fixes #6607