Fix anchoredPosition to position overlay in visible viewport even if out of screen scrollspace is available below the anchor element #577
Conversation
🦋 Changeset detectedLatest commit: 9e0416b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
pksjce
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where overlays were not properly positioned within the visible viewport when scrollable content existed below the anchor element. The fix ensures that when the clipping node is the document body, the viewport height is used instead of the body's potentially larger scrollable height for positioning calculations.
- Updated clipping rect calculation to use viewport height for document.body
- Added comprehensive test case for the bottom viewport positioning scenario
- Added changeset documentation for the patch release
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/anchored-position.ts | Modified getClippingRect to use window.innerHeight directly when clipping node is document.body |
| src/tests/anchored-position.test.ts | Added test case verifying overlay flips to top when positioned at bottom of viewport with scrollable content |
| .changeset/clean-coins-lie.md | Added changeset entry documenting the fix |
| height: | ||
| clippingNode === document.body | ||
| ? window.innerHeight | ||
| : Math.max(elemRect.height - borderTop - borderBottom, -Infinity), |
There was a problem hiding this comment.
The use of -Infinity as a fallback value in Math.max is unclear and could be confusing. Consider using 0 or a more descriptive constant, or add a comment explaining why -Infinity is the appropriate fallback.
| : Math.max(elemRect.height - borderTop - borderBottom, -Infinity), | |
| : Math.max(elemRect.height - borderTop - borderBottom, 0), // Fallback to 0 to ensure non-negative height |
Copilot uses AI. Check for mistakes.
pksjce
force-pushed
the
pk/anchoredposition
branch
from
9e0416b to
839e365
Compare
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@primer/behaviors": patch | |||
There was a problem hiding this comment.
I think its a patch because it fixes issue without breaking any other test cases
| // If the clipping node is document.body, use the viewport height instead of body height | ||
| height: | ||
| clippingNode === document.body | ||
| ? window.innerHeight | ||
| : Math.max(elemRect.height - borderTop - borderBottom, -Infinity), |
There was a problem hiding this comment.
Let's add this fix in a way that we can revert back to the previous implementation quickly if we find any issues 🙏
See https://github.com/github/primer/issues/5358#issuecomment-3059248179
|
This PR needs
|
Fixes https://github.com/github/primer/issues/5358