Replace polyfill with useAnchoredPosition#7725
Conversation
🦋 Changeset detectedLatest commit: b92d59f 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 |
|
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
1aab292 to
848c19c
Compare
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
| if (anchorElement) { | ||
| const overlayWidth = width ? parseInt(widthMap[width]) : null | ||
| const result = getDefaultPosition(anchorElement, overlayWidth) | ||
|
|
||
| if (result) { | ||
| currentOverlay.setAttribute('data-align', result.horizontal) | ||
| currentOverlay.style.setProperty('--anchored-overlay-anchor-offset-left', `${result.offset}px`) | ||
| } |
There was a problem hiding this comment.
This was added as a solution for https://github.com/github/primer/issues/6511. We calculate the required offset for the overlay to be where it can have the most space within the viewport.
This does mean we rely on JS + CSS, but this is a pretty lightweight way to do it. We'll only use JS when there isn't enough space for the overlay to exist, otherwise we rely fully on CSS anchor positioning
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/18469 |
liuliu-dev
left a comment
There was a problem hiding this comment.
I like the idea here :sparkle (the emoji selector issue here)
the offset approach to handle overflow is a smart fix!
| const leftOffset = Math.max(0, overlayWidth - rect.right + viewportMargin) | ||
| const rightOffset = Math.max(0, rect.left + overlayWidth - vw + viewportMargin) |
There was a problem hiding this comment.
I took a second to understand these 😅
Is leftOffset from ensuring overlayLeft >= viewportMargin, so viewportMargin - (rect.right - overlayWidth)>=0
and rightOffset from overlayRight <= vw - viewportMargin?
Could we add a short comment here explaining the formulas?
There was a problem hiding this comment.
You're right, it can be a bit confusing 😅 I added a comment to clarify! Thanks for the suggestion too!
|
🤖 Formatting issues have been automatically fixed and committed to this PR. |
Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>
Yup! We should be good to go! |
Part of https://github.com/github/primer/issues/6321
Closes https://github.com/github/primer/issues/6511, https://github.com/github/primer/issues/6538
Replaces the polyfill with our own JS-based anchoring as a fallback on browsers that do not support CSS anchor positioning.
Changelog
New
side='outside-right' andside='outside-left'.Changed
useAnchoredPositionhook.Rollout strategy
Testing & Reviewing
Merge checklist