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

Handle Nested Grid Dragging #6526

Merged
merged 14 commits into from
Oct 16, 2024
Merged

Conversation

seanparsons
Copy link
Contributor

@seanparsons seanparsons commented Oct 11, 2024

Problem:
Rearranging a grid inside a grid only works if dragging a cell of the nested grid.

Fix:
The main fix was the removal of a pointerEvents property which was inadvertently preventing the mousedown from triggering as it should.

But a secondary issue was discovered while testing that, which is that the mousedown is also not handled correctly in this case:
image
When the mousedown is triggered it selects the parent grid item, but this is too late as there's no handling present for the parent grid item as that gets initialised by the selection change. To fix this, ancestor grids have been added to the grids currently present so that the ancestor grids can catch the mouse down.

However, doing the above caused an issue of duplicated controls which broke some of our tests. To fix that combineApplicableControls was implemented to combine different instances of GridControls, thereby deduplicating some of the controls that would render.

Additional Fixes From Changes Requested:

  • To fix the issue of the grandparent element being highlighted, the check for the activeCell was extended to check if there were a sibling as well as checking the row and column.
  • To fix the issue of the animation twitching, the grids are ordered from topmost to bottommost so that the lowest (in the hierarchy) events that start the interaction trigger first.

Commit Details:

  • Implemented combineApplicableControls to collate disparate instances of GridControls.
  • controlsForGridPlaceholders has been slightly tweaked to cater for the change in the type of GridControlsProps and also to handle a new option suffix.
  • Removed pointerEvents setting which was bizarrely the cause of the original issue.
  • Removed spurious property.
  • GridControlsComponent now includes ancestor paths.
  • GridControlsComponent now sorts the grid paths by their depth, so that the bottommost event triggers fire over those that are ancestors of that element.

Manual Tests:
I hereby swear that:

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

- Implemented `combineApplicableControls` to collate disparate instances of
  `GridControls`.
- `controlsForGridPlaceholders` has been slightly tweaked to cater for the change
  in the type of `GridControlsProps` and also to handle a new option suffix.
- Removed `pointerEvents` setting which was bizarrely the cause of the original issue.
- Removed spurious property.
- `GridControlsComponent` now includes ancestor paths.
Copy link
Contributor

github-actions bot commented Oct 11, 2024

Try me

Copy link

relativeci bot commented Oct 11, 2024

#14821 Bundle Size — 57.97MiB (~+0.01%).

7215ecb(current) vs 916e8b9 master#14818(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Regression 1 regression
                 Current
#14821
     Baseline
#14818
Regression  Initial JS 40.95MiB(~+0.01%) 40.95MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 17.97% 0%
No change  Chunks 20 20
No change  Assets 22 22
No change  Modules 4151 4151
No change  Duplicate Modules 213 213
No change  Duplicate Code 27.34% 27.34%
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
#14821
     Baseline
#14818
Regression  JS 57.96MiB (~+0.01%) 57.96MiB
Improvement  HTML 7.37KiB (-0.25%) 7.39KiB

Bundle analysis reportBranch fix/handle-nested-grid-draggingProject dashboard


Generated by RelativeCIDocumentationReport issue

Copy link
Contributor

@balazsbajorics balazsbajorics left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I try to rearrange the child of a grid inside a grid, something goes wrong:
https://github.com/user-attachments/assets/0c749364-c674-4bdc-90f2-4500e2a623e7

in general if I have the child of a grid-in-a-grid selected, we shouldn't show visible grid outlines for the outer grid, as that grandparent element is irrelevant for the selected element

Copy link
Contributor

@balazsbajorics balazsbajorics left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

visually, everything should look like as it does on master:
https://github.com/user-attachments/assets/f5dddbc1-fefb-4957-ad22-0ba7c802a634

I tried the Try Me link, and when I have the inner rectangle selected, we shouldn't show the grid cells for the grandparent grid (as it is not relevant), the pink target cell outlines are hidden, and we also show too much of the blue helper background colors obscuring the content:
https://github.com/user-attachments/assets/4cfc3f4e-47ce-400e-840c-302993040c26

- For ancestor grids, supply a value for `controlsVisible` to hide
  the controls.
- Unwind a change to `addAllUniquelyBy`.
@seanparsons
Copy link
Contributor Author

visually, everything should look like as it does on master: https://github.com/user-attachments/assets/f5dddbc1-fefb-4957-ad22-0ba7c802a634

I tried the Try Me link, and when I have the inner rectangle selected, we shouldn't show the grid cells for the grandparent grid (as it is not relevant), the pink target cell outlines are hidden, and we also show too much of the blue helper background colors obscuring the content: https://github.com/user-attachments/assets/4cfc3f4e-47ce-400e-840c-302993040c26

Fixed in 2583a84.

@seanparsons seanparsons merged commit aa29342 into master Oct 16, 2024
15 checks passed
@seanparsons seanparsons deleted the fix/handle-nested-grid-dragging branch October 16, 2024 13:13
liady pushed a commit that referenced this pull request Dec 13, 2024
- Implemented `combineApplicableControls` to collate disparate instances
of `GridControls`.
- `controlsForGridPlaceholders` has been slightly tweaked to cater for
the change in the type of `GridControlsProps` and also to handle a new
option suffix.
- Removed `pointerEvents` setting which was bizarrely the cause of the
original issue.
- Removed spurious property.
- `GridControlsComponent` now includes ancestor paths.
- `GridControlsComponent` now sorts the grid paths by their depth, so
that the bottommost event triggers fire over those that are ancestors of
that element.
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