Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
58 changes: 52 additions & 6 deletions packages/react/src/SelectPanel/SelectPanel.examples.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ export const RepositionAfterLoading = () => {
const [loading, setLoading] = useState(true)

React.useEffect(() => {
// eslint-disable-next-line react-hooks/set-state-in-effect
// eslint-disable-next-line react-hooks/set-state-in-effect -- Reset loading state when panel closes
if (!open) setLoading(true)
window.setTimeout(() => {
if (open) {
Expand All @@ -369,7 +369,6 @@ export const RepositionAfterLoading = () => {

React.useEffect(() => {
if (!loading) {
// eslint-disable-next-line react-hooks/set-state-in-effect
setFilteredItems(items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())))
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down Expand Up @@ -405,7 +404,7 @@ export const SelectPanelRepositionInsideDialog = () => {
const [loading, setLoading] = useState(true)

React.useEffect(() => {
// eslint-disable-next-line react-hooks/set-state-in-effect
// eslint-disable-next-line react-hooks/set-state-in-effect -- Reset loading state when panel closes
if (!open) setLoading(true)
window.setTimeout(() => {
if (open) {
Expand All @@ -418,7 +417,6 @@ export const SelectPanelRepositionInsideDialog = () => {

React.useEffect(() => {
if (!loading) {
// eslint-disable-next-line react-hooks/set-state-in-effect
setFilteredItems(items.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())))
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand All @@ -438,14 +436,63 @@ export const SelectPanelRepositionInsideDialog = () => {
selected={selected}
onSelectedChange={setSelected}
onFilterChange={setFilter}
overlayProps={{anchorSide: 'outside-top'}}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unrelated change, but good to have here. The reposition story wasn't repositioning at all because we were always opening it above. Fixed the story, it works well.

message={filteredItems.length === 0 ? NoResultsMessage(filter) : undefined}
/>
</Stack>
</Dialog>
)
}

export const AutogrowAfterLoadingWithOutsideTopAnchor = () => {
const autogrowItems = [...items]

const [selected, setSelected] = React.useState<ItemInput[]>([autogrowItems[0], autogrowItems[1]])
const [open, setOpen] = useState(false)
const [filter, setFilter] = React.useState('')
const [filteredItems, setFilteredItems] = React.useState<typeof autogrowItems>([])
const [loading, setLoading] = useState(true)

React.useEffect(() => {
// eslint-disable-next-line react-hooks/set-state-in-effect -- Reset loading state when panel closes
if (!open) setLoading(true)
const timer = window.setTimeout(() => {
if (open) {
setFilteredItems(autogrowItems.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())))
setLoading(false)
}
}, 2000)

return () => window.clearTimeout(timer)
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [open])

React.useEffect(() => {
if (!loading) {
setFilteredItems(autogrowItems.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase())))
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [filter])

return (
<Stack direction="vertical" justify="space-between" style={{height: 'calc(100vh - 300px)', width: 'fit-content'}}>
<h1>Autogrow panel after loading with outside-top anchor</h1>
<SelectPanel
loading={loading}
title="Select labels"
placeholderText="Filter Labels"
open={open}
onOpenChange={setOpen}
items={filteredItems}
selected={selected}
onSelectedChange={setSelected}
onFilterChange={setFilter}
overlayProps={{anchorSide: 'outside-top'}}
message={filteredItems.length === 0 ? NoResultsMessage(filter) : undefined}
/>
</Stack>
)
}

export const WithDefaultMessage = () => {
const [selected, setSelected] = useState<ItemInput[]>(items.slice(1, 3))
const [filter, setFilter] = useState('')
Expand Down Expand Up @@ -624,7 +671,6 @@ export const VirtualizedConsumerSide = () => {
[open],
)

// eslint-disable-next-line react-hooks/incompatible-library
const virtualizer = useVirtualizer({
count: filteredItems.length,
getScrollElement: () => scrollContainer ?? null,
Expand Down
148 changes: 148 additions & 0 deletions packages/react/src/SelectPanel/SelectPanel.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2291,3 +2291,151 @@
expect(lastCall[2]?.displayInViewport).not.toBe(true)
})
})

/**
* Test suite for SelectPanel first-open sizing regression
* Issue: https://github.com/github/issues/issues/18331
*
* On first open with loading state, SelectPanel may render shorter than its content
* and show an unnecessary scrollbar. This occurs because position is calculated before
* items have loaded and rendered, using the spinner/loading state height rather than
* the final content height.
*/
describe('SelectPanel - First-Open Sizing with Loading State', () => {
const items: ItemInput[] = [
{id: '1', text: 'Item 1'},
{id: '2', text: 'Item 2'},
{id: '3', text: 'Item 3'},
{id: '4', text: 'Item 4'},
{id: '5', text: 'Item 5'},
{id: '6', text: 'Item 6'},
{id: '7', text: 'Item 7'},
{id: '8', text: 'Item 8'},
]

const TestComponentWithLoadingDelay = () => {
const [selected, setSelected] = React.useState<ItemInput | undefined>(items[0])
const [open, setOpen] = React.useState(false)
const [loading, setLoading] = React.useState(true)
const [visibleItems, setVisibleItems] = React.useState<ItemInput[]>([])

// Simulate items loading after a delay (typical async data fetch)
React.useEffect(() => {
if (!open) {
setLoading(true)
setVisibleItems([])
}

const timer = setTimeout(() => {
if (open) {
setVisibleItems(items)
setLoading(false)
}
}, 500) // 500ms delay simulates network/async operation

return () => clearTimeout(timer)
}, [open])

return (
<>
<SelectPanel
title="Select item"
open={open}
onOpenChange={setOpen}
onFilterChange={() => {}}
loading={loading}
items={visibleItems}
selected={selected}
onSelectedChange={setSelected}
height="large"
/>
<div data-testid="state">
open:{open} loading:{loading} itemsCount:{visibleItems.length}
</div>
</>
)
}

it('should measure overlay height correctly while loading', async () => {
const user = userEvent.setup()
render(<TestComponentWithLoadingDelay />)

const button = screen.getByRole('button', {name: /Select item/i})

Check failure on line 2363 in packages/react/src/SelectPanel/SelectPanel.test.tsx

View workflow job for this annotation

GitHub Actions / test (react-18)

[@primer/react (chromium)] src/SelectPanel/SelectPanel.test.tsx > SelectPanel - First-Open Sizing with Loading State > should measure overlay height correctly while loading

TestingLibraryElementError: Unable to find an accessible element with the role "button" and name `/Select item/i` Here are the accessible roles: button: Name "Item 1": <button aria-expanded="false" aria-haspopup="true" class="prc-Button-ButtonBase-Eb8-K" data-component="Button" data-loading="false" data-size="medium" data-variant="default" id=":r2ns:" tabindex="0" type="button" /> -------------------------------------------------- Ignored nodes: comments, script, style <body style="" > <div id="__primerPortalRoot__" style="position: absolute; top: 0px; left: 0px; width: 100%;" /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <div> <button aria-expanded="false" aria-haspopup="true" class="prc-Button-ButtonBase-Eb8-K" data-component="Button" data-loading="false" data-size="medium" data-variant="default" id=":r2ns:" tabindex="0" type="button" > <span class="prc-Button-ButtonContent-KZ5Bz" data-align="center" data-component="buttonContent" > <span class="prc-Button-Label-B1e2-" data-component="text" > Item 1 </span> </span> <span class="prc-Button-Visual-bL-6W prc-Button-VisualWrap-UmDs8" data-component="trailingAction" > <svg aria-hidden="true" class="octicon octicon-triangle-down" data-component="Octicon" display="inline-block" fill="currentColor" focusable="false" height="16" overflow="visible" style="vertical-align: text-bottom;" viewBox="0 0 16 16" width="16" > <path d="m4.427 7.427 3.396 3.396a.25.25 0 0 0 .354 0l3.396-3.396A.25.25 0 0 0 11.396 7H4.604a.25.25 0 0 0-.177.427Z" /> </svg> </span> </button> <div data-testid="state" > open: loading: itemsCount: 0 </div> </div> </body> ❯ Object.getElementError ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:339:18 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1206:24 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1185:16 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1228:18 ❯ getByRole src/SelectPanel/SelectPanel.test.tsx:2363:26

Check failure on line 2363 in packages/react/src/SelectPanel/SelectPanel.test.tsx

View workflow job for this annotation

GitHub Actions / test (react-19)

[@primer/react (chromium)] src/SelectPanel/SelectPanel.test.tsx > SelectPanel - First-Open Sizing with Loading State > should measure overlay height correctly while loading

TestingLibraryElementError: Unable to find an accessible element with the role "button" and name `/Select item/i` Here are the accessible roles: button: Name "Item 1": <button aria-expanded="false" aria-haspopup="true" class="prc-Button-ButtonBase-Eb8-K" data-component="Button" data-loading="false" data-size="medium" data-variant="default" id="_r_2ns_" tabindex="0" type="button" /> -------------------------------------------------- Ignored nodes: comments, script, style <body style="" > <div id="__primerPortalRoot__" style="position: absolute; top: 0px; left: 0px; width: 100%;" /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <div> <button aria-expanded="false" aria-haspopup="true" class="prc-Button-ButtonBase-Eb8-K" data-component="Button" data-loading="false" data-size="medium" data-variant="default" id="_r_2ns_" tabindex="0" type="button" > <span class="prc-Button-ButtonContent-KZ5Bz" data-align="center" data-component="buttonContent" > <span class="prc-Button-Label-B1e2-" data-component="text" > Item 1 </span> </span> <span class="prc-Button-Visual-bL-6W prc-Button-VisualWrap-UmDs8" data-component="trailingAction" > <svg aria-hidden="true" class="octicon octicon-triangle-down" data-component="Octicon" display="inline-block" fill="currentColor" focusable="false" height="16" overflow="visible" style="vertical-align: text-bottom;" viewBox="0 0 16 16" width="16" > <path d="m4.427 7.427 3.396 3.396a.25.25 0 0 0 .354 0l3.396-3.396A.25.25 0 0 0 11.396 7H4.604a.25.25 0 0 0-.177.427Z" /> </svg> </span> </button> <div data-testid="state" > open: loading: itemsCount: 0 </div> </div> </body> ❯ Object.getElementError ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:339:18 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1206:24 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1185:16 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1228:18 ❯ getByRole src/SelectPanel/SelectPanel.test.tsx:2363:26

// Open SelectPanel for the first time
await user.click(button)

// Wait for overlay to appear (should show loading spinner)
await waitFor(
() => {
expect(screen.getByText(/^Item 1$/)).toBeInTheDocument()
},
{timeout: 1000},
)

// Get overlay element
const overlay = document.querySelector('[data-testid="overlay"]') as HTMLElement | null

// Wait for items to load
await waitFor(
() => {
const state = screen.getByTestId('state')
expect(state).toHaveTextContent('loading:false')
},
{timeout: 1500},
)

// Get overlay measurements after loading
const clientHeightAfterLoading = overlay?.clientHeight
const scrollHeightAfterLoading = overlay?.scrollHeight

// The overlay should have enough height to fit all content without scrollbar
if (clientHeightAfterLoading && scrollHeightAfterLoading) {
const hasScrollbar = scrollHeightAfterLoading > clientHeightAfterLoading
expect(hasScrollbar).toBe(false)
}
})

it('should have consistent height between first and second open', async () => {
const user = userEvent.setup()
render(<TestComponentWithLoadingDelay />)

const button = screen.getByRole('button', {name: /Select item/i})

Check failure on line 2403 in packages/react/src/SelectPanel/SelectPanel.test.tsx

View workflow job for this annotation

GitHub Actions / test (react-18)

[@primer/react (chromium)] src/SelectPanel/SelectPanel.test.tsx > SelectPanel - First-Open Sizing with Loading State > should have consistent height between first and second open

TestingLibraryElementError: Unable to find an accessible element with the role "button" and name `/Select item/i` Here are the accessible roles: button: Name "Item 1": <button aria-expanded="false" aria-haspopup="true" class="prc-Button-ButtonBase-Eb8-K" data-component="Button" data-loading="false" data-size="medium" data-variant="default" id=":r2o1:" tabindex="0" type="button" /> -------------------------------------------------- Ignored nodes: comments, script, style <body style="" > <div id="__primerPortalRoot__" style="position: absolute; top: 0px; left: 0px; width: 100%;" /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <div> <button aria-expanded="false" aria-haspopup="true" class="prc-Button-ButtonBase-Eb8-K" data-component="Button" data-loading="false" data-size="medium" data-variant="default" id=":r2o1:" tabindex="0" type="button" > <span class="prc-Button-ButtonContent-KZ5Bz" data-align="center" data-component="buttonContent" > <span class="prc-Button-Label-B1e2-" data-component="text" > Item 1 </span> </span> <span class="prc-Button-Visual-bL-6W prc-Button-VisualWrap-UmDs8" data-component="trailingAction" > <svg aria-hidden="true" class="octicon octicon-triangle-down" data-component="Octicon" display="inline-block" fill="currentColor" focusable="false" height="16" overflow="visible" style="vertical-align: text-bottom;" viewBox="0 0 16 16" width="16" > <path d="m4.427 7.427 3.396 3.396a.25.25 0 0 0 .354 0l3.396-3.396A.25.25 0 0 0 11.396 7H4.604a.25.25 0 0 0-.177.427Z" /> </svg> </span> </button> <div data-testid="state" > open: loading: itemsCount: 0 </div> </div> </body> ❯ Object.getElementError ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:339:18 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1206:24 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1185:16 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1228:18 ❯ getByRole src/SelectPanel/SelectPanel.test.tsx:2403:26

Check failure on line 2403 in packages/react/src/SelectPanel/SelectPanel.test.tsx

View workflow job for this annotation

GitHub Actions / test (react-19)

[@primer/react (chromium)] src/SelectPanel/SelectPanel.test.tsx > SelectPanel - First-Open Sizing with Loading State > should have consistent height between first and second open

TestingLibraryElementError: Unable to find an accessible element with the role "button" and name `/Select item/i` Here are the accessible roles: button: Name "Item 1": <button aria-expanded="false" aria-haspopup="true" class="prc-Button-ButtonBase-Eb8-K" data-component="Button" data-loading="false" data-size="medium" data-variant="default" id="_r_2o1_" tabindex="0" type="button" /> -------------------------------------------------- Ignored nodes: comments, script, style <body style="" > <div id="__primerPortalRoot__" style="position: absolute; top: 0px; left: 0px; width: 100%;" /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <live-region /> <div> <button aria-expanded="false" aria-haspopup="true" class="prc-Button-ButtonBase-Eb8-K" data-component="Button" data-loading="false" data-size="medium" data-variant="default" id="_r_2o1_" tabindex="0" type="button" > <span class="prc-Button-ButtonContent-KZ5Bz" data-align="center" data-component="buttonContent" > <span class="prc-Button-Label-B1e2-" data-component="text" > Item 1 </span> </span> <span class="prc-Button-Visual-bL-6W prc-Button-VisualWrap-UmDs8" data-component="trailingAction" > <svg aria-hidden="true" class="octicon octicon-triangle-down" data-component="Octicon" display="inline-block" fill="currentColor" focusable="false" height="16" overflow="visible" style="vertical-align: text-bottom;" viewBox="0 0 16 16" width="16" > <path d="m4.427 7.427 3.396 3.396a.25.25 0 0 0 .354 0l3.396-3.396A.25.25 0 0 0 11.396 7H4.604a.25.25 0 0 0-.177.427Z" /> </svg> </span> </button> <div data-testid="state" > open: loading: itemsCount: 0 </div> </div> </body> ❯ Object.getElementError ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:339:18 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1206:24 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1185:16 ❯ ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js:1228:18 ❯ getByRole src/SelectPanel/SelectPanel.test.tsx:2403:26

// First open
await user.click(button)
await waitFor(() => {
const state = screen.getByTestId('state')
expect(state).toHaveTextContent('loading:false')
})

const overlay1 = document.querySelector('[data-testid="overlay"]')
const height1 = overlay1?.getBoundingClientRect().height

// Close
await user.click(button)
await waitFor(() => {
const state = screen.getByTestId('state')
expect(state).toHaveTextContent('open:false')
})

// Second open (loading state happens again)
await user.click(button)
await waitFor(
() => {
const state = screen.getByTestId('state')
expect(state).toHaveTextContent('loading:false')
},
{timeout: 1500},
)

const overlay2 = document.querySelector('[data-testid="overlay"]')
const height2 = overlay2?.getBoundingClientRect().height

// Heights should be very similar (allowing for minor variations)
if (height1 && height2) {
const difference = Math.abs(height1 - height2)
expect(difference).toBeLessThan(50) // Allow max 50px variance
}
})
})
Loading
Loading