Skip to content
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

FilteredActionList inherits height and maxHeight from its parent #1500

Merged
merged 5 commits into from
Oct 6, 2021
Merged
Show file tree
Hide file tree
Changes from 2 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
5 changes: 5 additions & 0 deletions .changeset/slow-badgers-relate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/components': patch
---

Fixed a bug where SelectPanel would not scroll with height:'auto'; maxHeight:'medium' passed to Overlay (https://github.com/github/primer/issues/333)
9 changes: 7 additions & 2 deletions src/FilteredActionList/FilteredActionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,12 @@ import {get} from '../constants'
import {useProvidedRefOrCreate} from '../hooks/useProvidedRefOrCreate'
import useScrollFlash from '../hooks/useScrollFlash'
import {useSSRSafeId} from '@react-aria/ssr'
import {SxProp} from '../sx'

export interface FilteredActionListProps extends Partial<Omit<GroupedListProps, keyof ListPropsBase>>, ListPropsBase {
export interface FilteredActionListProps
extends Partial<Omit<GroupedListProps, keyof ListPropsBase>>,
ListPropsBase,
SxProp {
loading?: boolean
placeholderText: string
filterValue?: string
Expand Down Expand Up @@ -57,6 +61,7 @@ export function FilteredActionList({
items,
textInputProps,
inputRef: providedInputRef,
sx,
...listProps
}: FilteredActionListProps): JSX.Element {
const [filterValue, setInternalFilterValue] = useProvidedStateOrCreate(externalFilterValue, undefined, '')
Expand Down Expand Up @@ -120,7 +125,7 @@ export function FilteredActionList({
useScrollFlash(scrollContainerRef)

return (
<Box display="flex" flexDirection="column" overflow="hidden">
<Box display="flex" flexDirection="column" overflow="hidden" sx={sx}>
<StyledHeader>
<TextInput
ref={inputRef}
Expand Down
26 changes: 13 additions & 13 deletions src/SelectPanel/SelectPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {FocusZoneHookSettings} from '../hooks/useFocusZone'
import {DropdownButton} from '../DropdownMenu'
import {ItemProps} from '../ActionList'
import {AnchoredOverlay, AnchoredOverlayProps} from '../AnchoredOverlay'
import Box from '../Box'
import {TextInputProps} from '../TextInput'
import {useProvidedStateOrCreate} from '../hooks/useProvidedStateOrCreate'
import {AnchoredOverlayWrapperAnchorProps} from '../AnchoredOverlay/AnchoredOverlay'
Expand Down Expand Up @@ -153,18 +152,19 @@ export function SelectPanel({
focusTrapSettings={focusTrapSettings}
focusZoneSettings={focusZoneSettings}
>
<Box display="flex" flexDirection="column" width="100%" height="100%">
<FilteredActionList
filterValue={filterValue}
onFilterChange={onFilterChange}
{...listProps}
role="listbox"
items={itemsToRender}
selectionVariant={isMultiSelectVariant(selected) ? 'multiple' : 'single'}
textInputProps={extendedTextInputProps}
inputRef={inputRef}
/>
</Box>
<FilteredActionList
filterValue={filterValue}
onFilterChange={onFilterChange}
{...listProps}
Copy link
Member

@smockle smockle Oct 6, 2021

Choose a reason for hiding this comment

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

Since the FilteredActionListProps interface now extends SxProp, someone may pass a listProps that includes sx, right? If they do, line 166 below will overwrite the sx value they pass. Would we want to merge (i.e. sx={{ ...listProps.sx, height: 'inherit', maxHeight: 'inherit' }})?

Copy link
Contributor Author

@jfuchs jfuchs Oct 6, 2021

Choose a reason for hiding this comment

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

I might have this wrong, but I think we're safe as is because the SelectPanelProps type doesn't include an sx prop, and listProps is just a "rest" destructuring of the SelectPanel props?

Copy link
Member

@smockle smockle Oct 6, 2021

Choose a reason for hiding this comment

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

@jfuchs I might be missing something, but it seems that SelectPanelProps does contain sx, via Omit<FilteredActionListProps, 'selectionVariant'>:

export type SelectPanelProps = SelectPanelBaseProps &
Omit<FilteredActionListProps, 'selectionVariant'> &

Screen shot showing the 'sx' property of 'SelectPanelProps'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, sorry — you're right, it is in there. Updated!

role="listbox"
items={itemsToRender}
selectionVariant={isMultiSelectVariant(selected) ? 'multiple' : 'single'}
textInputProps={extendedTextInputProps}
inputRef={inputRef}
// inheriting height and maxHeight ensures that the FilteredActionList is never taller
// than the Overlay (which would break scrolling the items)
sx={{height: 'inherit', maxHeight: 'inherit'}}
/>
</AnchoredOverlay>
)
}
Expand Down
53 changes: 53 additions & 0 deletions src/stories/SelectPanel.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -298,3 +298,56 @@ export function SelectPanelAboveTallBody(): JSX.Element {
)
}
SelectPanelAboveTallBody.storyName = 'SelectPanel, Above a Tall Body'

export function SelectPanelHeightAndScroll(): JSX.Element {
const longItems = [...items, ...items, ...items, ...items, ...items, ...items, ...items, ...items]
const [selectedA, setSelectedA] = React.useState<ItemInput | undefined>(longItems[0])
const [selectedB, setSelectedB] = React.useState<ItemInput | undefined>(longItems[0])
const [filter, setFilter] = React.useState('')
const filteredItems = longItems.filter(item => item.text.toLowerCase().startsWith(filter.toLowerCase()))
const [openA, setOpenA] = useState(false)
const [openB, setOpenB] = useState(false)

return (
<>
<h2>With height:medium</h2>
<SelectPanel
renderAnchor={({children, 'aria-labelledby': ariaLabelledBy, ...anchorProps}) => (
<DropdownButton aria-labelledby={` ${ariaLabelledBy}`} {...anchorProps}>
{children ?? 'Select Labels'}
</DropdownButton>
)}
placeholderText="Filter Labels"
open={openA}
onOpenChange={setOpenA}
items={filteredItems}
selected={selectedA}
onSelectedChange={setSelectedA}
onFilterChange={setFilter}
showItemDividers={true}
overlayProps={{height: 'medium'}}
/>
<h2>With height:auto, maxheight:medium</h2>
<SelectPanel
renderAnchor={({children, 'aria-labelledby': ariaLabelledBy, ...anchorProps}) => (
<DropdownButton aria-labelledby={` ${ariaLabelledBy}`} {...anchorProps}>
{children ?? 'Select Labels'}
</DropdownButton>
)}
placeholderText="Filter Labels"
open={openB}
onOpenChange={setOpenB}
items={filteredItems}
selected={selectedB}
onSelectedChange={setSelectedB}
onFilterChange={setFilter}
showItemDividers={true}
overlayProps={{
height: 'auto',
maxHeight: 'medium'
}}
/>
</>
)
}
SelectPanelHeightAndScroll.storyName = 'SelectPanel, Height and Scroll'