Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintain Grid Cell Pins. #6691

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Maintain Grid Cell Pins. #6691

merged 2 commits into from
Dec 3, 2024

Conversation

seanparsons
Copy link
Contributor

@seanparsons seanparsons commented Dec 2, 2024

Problem:
When dragging around grid children, currently the pins are reset to top/left regardless of what pins are set on the element.

Fix:
The first part of this work was to re-use the existing logic for updating the pins from other strategies that do maintain the pins, using the containing block of the element to calculate the pins against. However, this uncovered a flaw in that approach in that the element may at the same time be moved to a different set of coordinates within the grid. This results in the containing block of the element changing, which means that for one frame (each time it changes) the element jumps as the pins were calculated against the old containing block and the coordinates are also changed.

The solution for this secondary problem was to create an analogous set of elements with the new coordinates and retrieve the containing block from those. Since this is only done once per frame the cost is very small and doesn't affect the performance of the operation.

Commit Details:

  • Added printPinAsString utility function.
  • Implemented getGridRelativeContainingBlock to calculate a new containing block by
    building a minimal purely HTML reproduction of the grid so as to use the browser
    logic for the position and size of the containing block.
  • Refactored out getNewGridElementProps from the start of runGridChangeElementLocation
    so that it can be used to identify the new grid position on its own.
  • Refactored out getMoveCommandsForDrag from getMoveCommandsForSelectedElement.
  • Replaced gridChildAbsoluteMoveCommands with a call to getMoveCommandsForDrag
    in the Grid Absolute Move strategy.
  • Added GridElementChildContainingBlockKey for simplicity.

Manual Tests:
I hereby swear that:

  • I opened a hydrogen project and it loaded
  • I could navigate to various routes in Play mode

- Added `printPinAsString` utility function.
- Implemented `getGridRelativeContainingBlock` to calculate a new containing block by
  building a minimal purely HTML reproduction of the grid so as to use the browser
  logic for the position and size of the containing block.
- Refactored out `getNewGridElementProps` from the start of `runGridChangeElementLocation`
  so that it can be used to identify the new grid position on its own.
- Refactored out `getMoveCommandsForDrag` from `getMoveCommandsForSelectedElement`.
- Replaced `gridChildAbsoluteMoveCommands` with a call to `getMoveCommandsForDrag`
  in the Grid Absolute Move strategy.
- Added `GridElementChildContainingBlockKey` for simplicity.
Copy link
Contributor

github-actions bot commented Dec 2, 2024

Try me

Copy link

relativeci bot commented Dec 2, 2024

#15341 Bundle Size — 58.2MiB (+0.01%).

9d94985(current) vs d1d46c1 master#15340(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Regression 1 regression
                 Current
#15341
     Baseline
#15340
Regression  Initial JS 41.17MiB(+0.02%) 41.17MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 18.23% 18.67%
No change  Chunks 20 20
No change  Assets 22 22
No change  Modules 4189 4189
No change  Duplicate Modules 213 213
Change  Duplicate Code 27.24%(-0.04%) 27.25%
No change  Packages 477 477
No change  Duplicate Packages 70 70
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#15341
     Baseline
#15340
Regression  JS 58.19MiB (+0.01%) 58.19MiB
Improvement  HTML 9.4KiB (-0.21%) 9.42KiB

Bundle analysis reportBranch fix/maintain-grid-cell-pinsProject dashboard


Generated by RelativeCIDocumentationReport issue

@seanparsons seanparsons merged commit 6f299cf into master Dec 3, 2024
16 checks passed
@seanparsons seanparsons deleted the fix/maintain-grid-cell-pins branch December 3, 2024 16:55
ruggi added a commit that referenced this pull request Dec 4, 2024
**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
liady pushed a commit that referenced this pull request Dec 13, 2024
- Added `printPinAsString` utility function.
- Implemented `getGridRelativeContainingBlock` to calculate a new
containing block by
building a minimal purely HTML reproduction of the grid so as to use the
browser
  logic for the position and size of the containing block.
- Refactored out `getNewGridElementProps` from the start of
`runGridChangeElementLocation`
  so that it can be used to identify the new grid position on its own.
- Refactored out `getMoveCommandsForDrag` from
`getMoveCommandsForSelectedElement`.
- Replaced `gridChildAbsoluteMoveCommands` with a call to
`getMoveCommandsForDrag`
  in the Grid Absolute Move strategy.
- Added `GridElementChildContainingBlockKey` for simplicity.
liady pushed a commit that referenced this pull request Dec 13, 2024
**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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants