-
Notifications
You must be signed in to change notification settings - Fork 589
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
fix(SelectPanel): Correctly recalculate position on overflow #5562
Changes from 23 commits
80dfbfd
25920d6
a47034c
b863080
382e4d1
d792c50
dcf559d
0834641
ef2930d
59b9ce5
3e70f11
ed5fbbe
9a0fb5a
c2f2dcc
ef73210
81601de
08a9f9c
961eb39
ed98b1f
7c812cc
87201d2
7701ebd
6cb8394
5c5f4fc
971d1a9
02b81d4
35d58aa
d16252b
8c1e8cf
7615786
dc18438
c874e45
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 | ||
--- | ||
|
||
fix(SelectPanel): Correctly recalculate position on overflow |
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. RE: #5562 (comment) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
import type {Meta} from '@storybook/react' | ||
import React, {useState} from 'react' | ||
|
||
import {Button} from '../Button' | ||
import {AnchoredOverlay} from '.' | ||
import {Stack} from '../Stack' | ||
import {Dialog, Spinner} from '..' | ||
|
||
const meta = { | ||
title: 'Components/AnchoredOverlay/Dev', | ||
component: AnchoredOverlay, | ||
} satisfies Meta<typeof AnchoredOverlay> | ||
|
||
export default meta | ||
|
||
export const RepositionAfterContentGrows = () => { | ||
const [open, setOpen] = useState(false) | ||
|
||
const [loading, setLoading] = useState(true) | ||
|
||
React.useEffect(() => { | ||
window.setTimeout(() => { | ||
if (open) setLoading(false) | ||
}, 2000) | ||
}, [open]) | ||
|
||
return ( | ||
<Stack direction="vertical" justify="space-between" style={{height: 'calc(100vh - 200px)'}}> | ||
<div> | ||
What to expect: | ||
<ul> | ||
<li>The anchored overlay should open below the anchor (default position)</li> | ||
<li>After 2000ms, the amount of content in the overlay grows</li> | ||
<li>the overlay should reposition itself above the anchor so that it stays inside the window</li> | ||
</ul> | ||
</div> | ||
<AnchoredOverlay | ||
renderAnchor={props => ( | ||
<Button {...props} sx={{width: 'fit-content'}}> | ||
Button | ||
</Button> | ||
)} | ||
open={open} | ||
onOpen={() => setOpen(true)} | ||
onClose={() => { | ||
setOpen(false) | ||
setLoading(true) | ||
}} | ||
> | ||
{loading ? ( | ||
<> | ||
<Spinner /> | ||
loading for 2000ms | ||
</> | ||
) : ( | ||
<div style={{height: '300px'}}>content with 300px height</div> | ||
)} | ||
</AnchoredOverlay> | ||
</Stack> | ||
) | ||
} | ||
|
||
export const RepositionAfterContentGrowsWithinDialog = () => { | ||
const [open, setOpen] = useState(false) | ||
|
||
const [loading, setLoading] = useState(true) | ||
|
||
React.useEffect(() => { | ||
window.setTimeout(() => { | ||
if (open) setLoading(false) | ||
}, 2000) | ||
}, [open]) | ||
|
||
return ( | ||
<Dialog onClose={() => {}}> | ||
<Stack direction="vertical" justify="space-between" style={{height: 'calc(100vh - 300px)'}}> | ||
<div> | ||
What to expect: | ||
<ul> | ||
<li>The anchored overlay should open below the anchor (default position)</li> | ||
<li>After 2000ms, the amount of content in the overlay grows</li> | ||
<li>the overlay should reposition itself above the anchor so that it stays inside the window</li> | ||
</ul> | ||
</div> | ||
<AnchoredOverlay | ||
renderAnchor={props => ( | ||
<Button {...props} sx={{width: 'fit-content'}}> | ||
Button | ||
</Button> | ||
)} | ||
open={open} | ||
onOpen={() => setOpen(true)} | ||
onClose={() => { | ||
setOpen(false) | ||
setLoading(true) | ||
}} | ||
> | ||
{loading ? ( | ||
<> | ||
<Spinner /> | ||
loading for 2000ms | ||
</> | ||
) : ( | ||
<div style={{height: '300px'}}>content with 300px height</div> | ||
)} | ||
</AnchoredOverlay> | ||
</Stack> | ||
</Dialog> | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,6 +89,10 @@ interface AnchoredOverlayBaseProps extends Pick<OverlayProps, 'height' | 'width' | |
* If `preventOverflow` is `true`, the width of the `Overlay` will not be adjusted. | ||
*/ | ||
preventOverflow?: boolean | ||
/** | ||
* If true, the overlay will attempt to prevent position shifting when sitting at the top of the anchor. | ||
*/ | ||
pinPosition?: boolean | ||
} | ||
|
||
export type AnchoredOverlayProps = AnchoredOverlayBaseProps & | ||
|
@@ -112,11 +116,12 @@ export const AnchoredOverlay: React.FC<React.PropsWithChildren<AnchoredOverlayPr | |
overlayProps, | ||
focusTrapSettings, | ||
focusZoneSettings, | ||
side = 'outside-bottom', | ||
side = overlayProps?.['anchorSide'] || 'outside-bottom', | ||
align = 'start', | ||
alignmentOffset, | ||
anchorOffset, | ||
className, | ||
pinPosition, | ||
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. Should we add this to 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. Yes! Done!! |
||
preventOverflow = true, | ||
}) => { | ||
const anchorRef = useProvidedRefOrCreate(externalAnchorRef) | ||
|
@@ -155,6 +160,7 @@ export const AnchoredOverlay: React.FC<React.PropsWithChildren<AnchoredOverlayPr | |
{ | ||
anchorElementRef: anchorRef, | ||
floatingElementRef: overlayRef, | ||
pinPosition, | ||
side, | ||
align, | ||
alignmentOffset, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' | |
export interface AnchoredPositionHookSettings extends Partial<PositionSettings> { | ||
floatingElementRef?: React.RefObject<Element> | ||
anchorElementRef?: React.RefObject<Element> | ||
pinPosition?: boolean | ||
camertron marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
|
@@ -30,22 +31,50 @@ export function useAnchoredPosition( | |
const floatingElementRef = useProvidedRefOrCreate(settings?.floatingElementRef) | ||
const anchorElementRef = useProvidedRefOrCreate(settings?.anchorElementRef) | ||
const [position, setPosition] = React.useState<AnchorPosition | undefined>(undefined) | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const [_, setPrevHeight] = React.useState<number | undefined>(undefined) | ||
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. used to force the height of the selectPanel when filtered and it wants to shrink but it's anchored at the top (we want it to stay at the top and lock the height) |
||
|
||
const updatePosition = React.useCallback( | ||
() => { | ||
if (floatingElementRef.current instanceof Element && anchorElementRef.current instanceof Element) { | ||
setPosition(getAnchoredPosition(floatingElementRef.current, anchorElementRef.current, settings)) | ||
const newPosition = getAnchoredPosition(floatingElementRef.current, anchorElementRef.current, settings) | ||
setPosition(prev => { | ||
if ( | ||
settings?.pinPosition && | ||
prev && | ||
['outside-top', 'inside-top'].includes(prev.anchorSide) && | ||
(prev.anchorSide !== newPosition.anchorSide || prev.top < newPosition.top) | ||
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. if the selectPanel is anchored to the top and it used to sit higher on the page.... 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. Could we wrap this in a function and give it some comments so we know what's going on when we come back to the code in a few months? 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. Done! |
||
) { | ||
const anchorTop = anchorElementRef.current?.getBoundingClientRect().top ?? 0 | ||
if (anchorTop > (floatingElementRef.current?.clientHeight ?? 0)) { | ||
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. if the anchorTop is bigger than the selectPanel's height, that means there is plenty of space for the selectPanel to stay at the top 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. Same question: maybe wrap in a function with a nice, intention-revealing name and some comments? 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. Done! |
||
setPrevHeight(prevHeight => { | ||
if (prevHeight && prevHeight > (floatingElementRef.current?.clientHeight ?? 0)) { | ||
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. re-establish the height if it used to be bigger and is now attempting to shrink |
||
requestAnimationFrame(() => { | ||
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. using requestAnimationFrame here to prevent a resizeObserver loop here due to us changing the element's height, but the observer listening for changes in the element... |
||
;(floatingElementRef.current as HTMLElement).style.height = `${prevHeight}px` | ||
}) | ||
} else { | ||
prev = newPosition | ||
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. edgecase where the selectPanel is actually growing instead of shrinking, in this case we do want to allow it to recalculate/reposition |
||
} | ||
return prevHeight | ||
}) | ||
return prev | ||
} | ||
} | ||
return newPosition | ||
}) | ||
} else { | ||
setPosition(undefined) | ||
} | ||
setPrevHeight(floatingElementRef.current?.clientHeight) | ||
}, | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
[floatingElementRef, anchorElementRef, ...dependencies], | ||
) | ||
|
||
useLayoutEffect(updatePosition, [updatePosition]) | ||
|
||
useResizeObserver(updatePosition) | ||
useResizeObserver(updatePosition) // watches for changes in window size | ||
useResizeObserver(updatePosition, floatingElementRef as React.RefObject<HTMLElement>) // watches for changes in floating element size | ||
|
||
return { | ||
floatingElementRef, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,5 +59,5 @@ export function useResizeObserver<T extends HTMLElement>( | |
} | ||
|
||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [target, ...depsArray]) | ||
}, [target?.current, ...depsArray]) | ||
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 know you originally moved line 25 outside of the hook and put the dependency on the |
||
} |
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.
No idea what these VRT changes are about. From the pictures it looks like the focus outline color is changing but I tried to reproduce locally and have been unable to (this is a dev only story). It also looks like the new outline more closely matches production 🤷♀️
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.
Ugh yeah this has been happening more lately... maybe b/c of CSS modules??
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.
I really don't know, it was failing on both flags (on and off)