Fix issues with DragOverlay measuring #427
Conversation
🦋 Changeset detectedLatest commit: d7a4949 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
clauderic
force-pushed
the
fix-drag-overlay-measuring
branch
from
de29ae1
to
9a0815c
Compare
|
Size Change: -231 B (0%) Total Size: 61.4 kB
ℹ️ View Unchanged
|
clauderic
force-pushed
the
fix-drag-overlay-measuring
branch
from
9a0815c
to
a1c1a5e
Compare
| droppableRects, | ||
| over, | ||
| recomputeLayouts, | ||
| willRecomputeLayouts, | ||
| } = useDndContext(); | ||
| const containerId = useUniqueId(ID_PREFIX, id); | ||
| const useDragOverlay = Boolean(overlayNode.rect !== null); | ||
| const useDragOverlay = Boolean(dragOverlay.rect !== null); |
There was a problem hiding this comment.
This throws an error on my branch as dragOverlay is undefined.
There was a problem hiding this comment.
Yeah this happens sometimes with breaking TypeScript changes across packages, just re-run yarn start and it'll fix itself (eventually). Haven't had time to prioritize upgrading development tooling but it's on the roadmap.
There was a problem hiding this comment.
Yep, kicking it a few times did the trick.
|
I converted this PR to draft because I still need to make a couple tweaks, but it should be good enough for you to give it a try |
|
Looks good on my branch! |
|
Good to hear. Out of curiosity, are you positioning the drag overlay child with I'm a bit worried about the amount of complexity it adds to try and support measuring the drag overlay child using |
We add |
|
That counts towards |
|
For my curiosity's sake, which step of computation are you considering shifting? Shouldn't the final rect include transformations and therefore make |
|
There are a lot of timing-related concerns with regards to measuring elements that have transforms applied. It requires elaborate orchestration to make sure you measure an element at the appropriate point in time, either before or after transforms are updated. You can see issues with the current strategy if you navigate to the Multiple containers story and sort with the keyboard. The timing of when the DragOverlay node is measured conflicts with the collision detection logic (there's a race condition between the two). Before.-.DragOverlay.measured.with.transforms.applied.mp4These issues aren't impossible to solve, but I'd rather keep the code simple and reliable for now and improve on it later, even if it means not supporting DragOverlay children that are positioned using transforms (for the time being). After.-.DragOverlay.transform.agnostic.measuring.strategy.mp4 |
This approach is transform agnostic to keep things simple and reliable. It can be improved later down the road.
This PR follows-up on #408 and #426
It fixes a number of issues with the measuring strategy for the DragOverlay. The main change is that we need to subtract the transform applied on the DragOverlay node when measuring it, otherwise the returned measurement includes the transforms, which means that the next time we compute the collision rect (by adding the transform to the drag overlay position), the newly calculated collision rect has double transforms, which is incorrect.
@wmain can you test this PR against your branch and confirm that it fixes the issues you were seeing with the original solution?