Skip to content

experimental/SelectPanel: Update anchored position after showModal#4396

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.

Sign up for GitHub

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

Closed
siddharthkp wants to merge 5 commits into from
Closed

experimental/SelectPanel: Update anchored position after showModal #4396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8456564
add repro for problem
siddharthkp Mar 15, 2024
7e01470
update position programmatically once dialog is open
siddharthkp Mar 15, 2024
2761316
update delay to match 60Hz
siddharthkp Mar 15, 2024
0dc5ca9
Create proud-walls-study.md
siddharthkp Mar 18, 2024
15c0853
Merge branch 'main' into drafts-selectpanel-anchored-position-fix
siddharthkp Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/proud-walls-study.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

exerimental/SelectPanel: Fix anchored position when close to the edge of browser window
2 changes: 2 additions & 0 deletions packages/react/src/Overlay/Overlay.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -71,7 +71,9 @@ export const StyledOverlay = styled.div<StyledOverlayProps>`
border-radius: 12px;
overflow: ${props => (props.overflow ? props.overflow : 'hidden')};
animation: overlay-appear ${animationDuration}ms ${get('animation.easeOutCubic')};
animation-fill-mode: forwards;

opacity: 0;

@siddharthkpsiddharthkpMar 15, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewer: Need to add this if we want to delay animation

@keyframes overlay-appear {
0% {
opacity: 0;
Expand Down
31 changes: 31 additions & 0 deletions packages/react/src/drafts/SelectPanel2/SelectPanel.examples.stories.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -847,6 +847,37 @@ export const NestedSelection = () => {
)
}

export const ReproAtTheEdge = () => {
const [selectedTag, setSelectedTag] = React.useState<string>()

const onSubmit = () => {
if (!selectedTag) return
data.ref = selectedTag // pretending to persist changes
}

const itemsToShow = data.tags

return (
<>
<h1>SelectPanel at the edge</h1>

<Box sx={{display: 'flex', justifyContent: 'right'}}>
<SelectPanel title="Choose a tag" selectionVariant="instant" onSubmit={onSubmit}>
<SelectPanel.Button leadingVisual={TagIcon}>{selectedTag || 'Choose a tag'}</SelectPanel.Button>

<ActionList>
{itemsToShow.map(tag => (
<ActionList.Item key={tag.id} onSelect={() => setSelectedTag(tag.id)} selected={selectedTag === tag.id}>
{tag.name}
</ActionList.Item>
))}
</ActionList>
</SelectPanel>
</Box>
</>
)
}

export const CreateNewRow = () => {
const initialSelectedLabels = data.issue.labelIds // mock initial state: has selected labels
const [selectedLabelIds, setSelectedLabelIds] = React.useState<string[]>(initialSelectedLabels)
Expand Down
41 changes: 27 additions & 14 deletions packages/react/src/drafts/SelectPanel2/SelectPanel.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -164,11 +164,31 @@ const Panel: React.FC<SelectPanelProps> = ({
/* Dialog */
const dialogRef = React.useRef<HTMLDialogElement>(null)

/* Anchored position */

@siddharthkpsiddharthkpMar 15, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewer: This function already existed, just moved it above

const {position, updatePosition} = useAnchoredPosition(
{
anchorElementRef: anchorRef,
floatingElementRef: dialogRef,
side: 'outside-bottom',
align: 'start',
},
[
/*
Because we call dialog.showModal() to open the dialog,
we need to wait until the browser opens the dialog (giving it dimensions)
before updating position. (it is 0px by 0px until opened)
This is why we explicitly call updatePosition instead of using dependencies.
*/
],
)

// sync dialog open state (imperative) with internal component state
React.useEffect(() => {
if (internalOpen) dialogRef.current?.showModal()
else if (dialogRef.current?.open) dialogRef.current.close()
}, [internalOpen])
if (internalOpen) {
dialogRef.current?.showModal()
updatePosition() // update the position once the dialog is open
} else if (dialogRef.current?.open) dialogRef.current.close()

@keithamuskeithamusMar 21, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be inlined

Suggested change
}elseif(dialogRef.current?.open)dialogRef.current.close()
}elsedialogRef.current?.close()

}, [internalOpen, updatePosition])

// dialog handles Esc automatically, so we have to sync internal state
// but it doesn't call onCancel, so have another effect for that!
Expand DownExpand Up@@ -202,17 +222,6 @@ const Panel: React.FC<SelectPanelProps> = ({
[internalOpen],
)

/* Anchored */
const {position} = useAnchoredPosition(
{
anchorElementRef: anchorRef,
floatingElementRef: dialogRef,
side: 'outside-bottom',
align: 'start',
},
[internalOpen, anchorRef.current, dialogRef.current],
)

/*
We want to cancel and close the panel when user clicks outside.
See decision log: https://github.com/github/primer/discussions/2614#discussioncomment-8544561
Expand All@@ -233,6 +242,10 @@ const Panel: React.FC<SelectPanelProps> = ({
maxHeight={maxHeight}
data-variant={currentVariant}
sx={{
// to avoid a visible position shift, we delay the overlay animating-in
// to wait until the correct position is set (see useAnchoredPosition above for more)
animationDelay: '16ms',

@siddharthkpsiddharthkpMar 15, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewer: This isn't scientific at all, but we need to pick a number, so based it off a 60Hz refresh rate 😅

From https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame,

The frequency of calls to the callback function will generally match the display refresh rate. The most common refresh rate is 60hz, (60 cycles/frames per second), though 75hz, 120hz, and 144hz are also widely used. requestAnimationFrame() calls are paused in most browsers when running in background tabs or hidden iframes, in order to improve performance and battery life.

@siddharthkpsiddharthkpMar 15, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative approach would be to "watch" for dialog open. Either in a recursive timeout or intersection observer, but that felt overkill!


'--max-height': heightMap[maxHeight],
// reset dialog default styles
border: 'none',
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/hooks/useAnchoredPosition.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -26,6 +26,7 @@ export function useAnchoredPosition(
floatingElementRef: React.RefObject<Element>
anchorElementRef: React.RefObject<Element>
position: AnchorPosition | undefined
updatePosition: () => void
} {
const floatingElementRef = useProvidedRefOrCreate(settings?.floatingElementRef)
const anchorElementRef = useProvidedRefOrCreate(settings?.anchorElementRef)
Expand All@@ -51,5 +52,6 @@ export function useAnchoredPosition(
floatingElementRef,
anchorElementRef,
position,
updatePosition,
}
}