-
Notifications
You must be signed in to change notification settings - Fork 656
useAnchoredPosition: recalculate position on scroll #7652
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
base: main
Are you sure you want to change the base?
Changes from all commits
64f8eeb
702edd0
8d19b43
fbacefb
d5f2bb7
d988de1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@primer/react': patch | ||
| --- | ||
|
|
||
| useAnchoredPosition: recalculate overlay position when any scrollable ancestor (or the window) is scrolled. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,27 @@ import {useProvidedRefOrCreate} from './useProvidedRefOrCreate' | |
| import {useResizeObserver} from './useResizeObserver' | ||
| import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' | ||
|
|
||
| /** | ||
| * Returns all scrollable ancestor elements of the given element, plus the window. | ||
| * An element is scrollable if its computed overflow/overflow-x/overflow-y is | ||
| * 'auto', 'scroll', or 'overlay'. | ||
| */ | ||
| function getScrollableAncestors(element: Element): Array<Element | Window> { | ||
| const scrollables: Array<Element | Window> = [] | ||
| let current = element.parentElement | ||
| while (current) { | ||
| const style = getComputedStyle(current) | ||
| const overflowY = style.overflowY | ||
| const overflowX = style.overflowX | ||
| if (/auto|scroll|overlay/.test(overflowY) || /auto|scroll|overlay/.test(overflowX)) { | ||
| scrollables.push(current) | ||
| } | ||
| current = current.parentElement | ||
| } | ||
| scrollables.push(window) | ||
| return scrollables | ||
| } | ||
|
|
||
| export interface AnchoredPositionHookSettings extends Partial<PositionSettings> { | ||
| floatingElementRef?: React.RefObject<Element | null> | ||
| anchorElementRef?: React.RefObject<Element | null> | ||
|
|
@@ -100,6 +121,37 @@ export function useAnchoredPosition( | |
| useResizeObserver(updatePosition) // watches for changes in window size | ||
| useResizeObserver(updatePosition, floatingElementRef as React.RefObject<HTMLElement | null>) // watches for changes in floating element size | ||
|
|
||
| // Recalculate position when any scrollable ancestor of the anchor scrolls. | ||
| // Uses requestAnimationFrame to avoid layout thrashing during scroll. | ||
| React.useEffect(() => { | ||
| const anchorEl = anchorElementRef.current | ||
| if (!anchorEl) return | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Random question - could we only run this when the overlay is active? I'm assuming no, but wanted to ask 😁 Same goes for the scroll event; if we couldn't return early here if the overlay active, would we only want to add the scroll event if the overlay is open?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked into this - we could add an option but then we'd need to implement it in loads of places, or there's a but of a hacky fix that relies on a Suggest given this fix should be relatively short lived, we leave it for now?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense! Let's leave it as-is then. |
||
|
|
||
|
Comment on lines
+126
to
+129
|
||
| let rafId: number | null = null | ||
| const handleScroll = () => { | ||
| if (rafId !== null) return | ||
| rafId = requestAnimationFrame(() => { | ||
| rafId = null | ||
| updatePosition() | ||
| }) | ||
| } | ||
|
|
||
| const scrollables = getScrollableAncestors(anchorEl) | ||
| for (const scrollable of scrollables) { | ||
| // eslint-disable-next-line github/prefer-observers -- IntersectionObserver cannot detect continuous scroll position changes needed for repositioning | ||
| scrollable.addEventListener('scroll', handleScroll) | ||
| } | ||
|
Comment on lines
+139
to
+143
|
||
|
|
||
| return () => { | ||
| for (const scrollable of scrollables) { | ||
| scrollable.removeEventListener('scroll', handleScroll) | ||
| } | ||
| if (rafId !== null) { | ||
| cancelAnimationFrame(rafId) | ||
| } | ||
| } | ||
| }, [anchorElementRef, updatePosition]) | ||
|
|
||
| return { | ||
| floatingElementRef, | ||
| anchorElementRef, | ||
|
|
||
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.
Does this need to run when the
AnchoredOverlayis closed? I'm wondering if there are any performance implications, but I doubt it, as nothing here stands out as being slow.