-
Notifications
You must be signed in to change notification settings - Fork 174
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
Conversation
- 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.
#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
Bundle size by type
Bundle analysis report Branch fix/handle-nested-grid-dragging Project dashboard Generated by RelativeCI Documentation Report issue |
There was a problem hiding this 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
There was a problem hiding this 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`.
Fixed in 2583a84. |
- Fixed issue with pink outlines not showing up. - Fixed subtle bug with `addAllUniquelyBy`.
- 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.
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:

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 ofGridControls
, thereby deduplicating some of the controls that would render.Additional Fixes From Changes Requested:
activeCell
was extended to check if there were a sibling as well as checking the row and column.Commit Details:
combineApplicableControls
to collate disparate instances ofGridControls
.controlsForGridPlaceholders
has been slightly tweaked to cater for the change in the type ofGridControlsProps
and also to handle a new option suffix.pointerEvents
setting which was bizarrely the cause of the original issue.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: