fix(core): track overlay on inner-container scroll (#913)#1690
Open
aalimovs wants to merge 2 commits into
Open
fix(core): track overlay on inner-container scroll (#913)#1690aalimovs wants to merge 2 commits into
aalimovs wants to merge 2 commits into
Conversation
The selection/hover overlay is portaled outside any scrollable DropZone and positioned absolutely against the portal target. getStyle() compensated for scroll using getDeepScrollPosition(el), which summed *every* ancestor's scroll — including intermediate scroll containers between the item and the portal target. That term cancelled the item's getBoundingClientRect() movement, so the overlay froze whenever an inner container (e.g. a DropZone with overflow:auto) was scrolled, even though page scroll worked. Compensate for only the portal target's own scroll instead: - non-iframe: the [data-puck-preview] container's own scrollLeft/Top - iframe / document-body portal: the document's own scrollX/Y Also re-sync the overlay on scroll while hovering (not just when selected or dragging) so hovered overlays track inner-container scroll, while keeping the continuous rAF layout-shift tracker gated to selected/dragging to avoid a per-frame loop on hover. Removes the now-unused get-deep-scroll-position helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@aalimovs is attempting to deploy a commit to the Puck Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR fixes overlay positioning for selected/hovered components when scrolling within inner overflow containers (notably horizontally overflowing DropZones), addressing #913 by compensating only for the portal target’s own scroll rather than summing all ancestor scroll offsets.
Changes:
- Updated
DraggableComponentoverlaygetStyle()to compensate for portal target scroll only (preview container scroll in non-iframe mode; document scroll in iframe/body-portal mode). - Ensured hovered (and indicative-hovered) overlays re-sync on scroll/resize without enabling the continuous per-frame rAF loop (reserved for selected/dragging).
- Removed the unused internal
get-deep-scroll-positionhelper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/core/lib/get-deep-scroll-position.ts | Removed unused deep ancestor scroll-summing helper now that overlay positioning no longer needs it. |
| packages/core/components/DraggableComponent/index.tsx | Reworked overlay scroll compensation logic and expanded scroll/resize resync to hovered states while keeping rAF usage limited to selected/dragging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
@shannonhochkins would you mind checking this against your Home Assistant use-case? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Not gonna lie, thank you Claude.
However, this was extensively manually reviewed and tested, for both iframe and non-iframe setup.
Happy to clean up comments if this is approved.
Closes #913.
Description
Fixes issue with horizontal scrolling when a component is selected or hovered, exactly like on video in #913.
The overlay is portaled outside the scrolling container, but
getStyle()was compensating withgetDeepScrollPosition(el), which summed the scroll of every ancestor, including the innerDropZone. That extra amount cancelled out the item's movement, so the overlay froze.Now it only accounts for the scroll of the element the overlay is actually attached to:
[data-puck-preview]container's own scrollChanges made
getStyle()compensates for only the portal target's own scroll instead of summing every ancestor.get-deep-scroll-positionhelper (internal-only).How to test
See #913 video.