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

Selectpanel2: Remove SelectPanel.ActionList API #3764

Merged
merged 43 commits into from
Oct 2, 2023
Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
080351c
remove sort + divider
siddharthkp Sep 20, 2023
445e150
reorder state to be more cohesive
siddharthkp Sep 20, 2023
3b36ae0
clean up suspended story
siddharthkp Sep 20, 2023
f14d727
clean up C and D
siddharthkp Sep 20, 2023
c7ed2d7
A: sort items to show initially selected ones first
siddharthkp Sep 20, 2023
6296f32
add event.preventDefault by default
siddharthkp Sep 21, 2023
7350510
add initial sorting function to stories
siddharthkp Sep 21, 2023
a5dbe92
add missing props for D with use transition
siddharthkp Sep 21, 2023
2f12c35
Replace SelectPanel.Heading with title
siddharthkp Sep 22, 2023
8413f74
add default footer
siddharthkp Sep 22, 2023
a7208c3
add minimal story
siddharthkp Sep 22, 2023
39f6779
oops, it's not possible to have query
siddharthkp Sep 22, 2023
7689c7f
Merge branch 'main' into drafts-replace-heading-with-title
siddharthkp Sep 22, 2023
7ce02ba
clean up story with tabs
siddharthkp Sep 25, 2023
796effe
Merge branch 'drafts-replace-heading-with-title' of github.com:primer…
siddharthkp Sep 25, 2023
88f27e5
Add option for external anchor
siddharthkp Sep 25, 2023
6fc2508
improve custom events story
siddharthkp Sep 25, 2023
dd17537
story: bring back filter for Filter story
siddharthkp Sep 25, 2023
d774dd8
Merge branch 'drafts-replace-heading-with-title' into drafts-external…
siddharthkp Sep 25, 2023
5dd2853
rename story
siddharthkp Sep 25, 2023
211d5ba
remove the need for SelectPanel.ActionList
siddharthkp Sep 25, 2023
4720653
add selection variable by default
siddharthkp Sep 25, 2023
ba07364
add list and item role automatically
siddharthkp Sep 25, 2023
a4197ad
oops, unused imports
siddharthkp Sep 25, 2023
1420edc
reduce scope
siddharthkp Sep 25, 2023
be44380
update ActionList snapshot
siddharthkp Sep 25, 2023
d34ca0a
Merge branch 'main' into drafts-selectpanel-simplify-actionlist
siddharthkp Sep 25, 2023
0d08c01
fix bad merge, sorry!
siddharthkp Sep 25, 2023
a7fa2d9
missed a spot!
siddharthkp Sep 25, 2023
9bf52b9
remove unrelated change
siddharthkp Sep 25, 2023
6753461
update snapshots
siddharthkp Sep 25, 2023
6011cc6
bring back explicit footer for suspense
siddharthkp Sep 26, 2023
40d3c57
Add warning to ActionMenu story
siddharthkp Sep 26, 2023
8e00e7f
undefined can't be a string!
siddharthkp Sep 26, 2023
02901a8
test(vrt): update snapshots
siddharthkp Sep 26, 2023
279e846
found an easier way to style
siddharthkp Sep 26, 2023
eed8da1
Merge branch 'main' into drafts-selectpanel-simplify-actionlist
siddharthkp Sep 27, 2023
f046646
Revert "test(vrt): update snapshots"
siddharthkp Sep 27, 2023
9f8cf08
Merge branch 'drafts-selectpanel-footer-fix' into drafts-selectpanel-…
siddharthkp Sep 27, 2023
d1c872b
Empty message should be out of ActionList
siddharthkp Sep 27, 2023
283f800
footer outside BOx
siddharthkp Sep 27, 2023
332e132
Merge branch 'main' into drafts-selectpanel-simplify-actionlist
siddharthkp Sep 27, 2023
7e8b46a
Merge branch 'main' into drafts-selectpanel-simplify-actionlist
siddharthkp Oct 2, 2023
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
2 changes: 1 addition & 1 deletion src/ActionList/ActionListContainerContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {AriaRole} from '../utils/types'
type ContextProps = {
container?: string
listRole?: AriaRole
selectionVariant?: 'single' | 'multiple' // TODO: Remove after DropdownMenu2 deprecation
Copy link
Member Author

Choose a reason for hiding this comment

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

i guess we're not removing this 😇

selectionVariant?: 'single' | 'multiple'
selectionAttribute?: 'aria-selected' | 'aria-checked'
listLabelledBy?: string
// This can be any function, we don't know anything about the arguments
Expand Down
9 changes: 8 additions & 1 deletion src/ActionList/Item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
trailingVisual: TrailingVisual,
description: Description,
})
const {variant: listVariant, showDividers, selectionVariant: listSelectionVariant} = React.useContext(ListContext)
const {
variant: listVariant,
role: listRole,
showDividers,
selectionVariant: listSelectionVariant,
} = React.useContext(ListContext)
const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext)
const {container, afterSelect, selectionAttribute} = React.useContext(ActionListContainerContext)

Expand All @@ -65,6 +70,8 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
if (selectionVariant === 'single') itemRole = 'menuitemradio'
else if (selectionVariant === 'multiple') itemRole = 'menuitemcheckbox'
else itemRole = 'menuitem'
} else if (container === 'SelectPanel' && listRole === 'listbox') {
if (selectionVariant !== undefined) itemRole = 'option'
Copy link
Member Author

@siddharthkp siddharthkp Sep 25, 2023

Choose a reason for hiding this comment

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

This looks like a potentially breaking change, buuut, we never override props.role if provided. So we should be good :)

role: props.role || itemRole

const containerProps = _PrivateItemWrapper ? {role: role || itemRole ? 'none' : undefined} : menuItemProps

Side note: We could probably apply these without a container as well, but I'd like to test that out separately in dotcom. Keeping the scope tight in this PR.

}

const {theme} = useTheme()
Expand Down
136 changes: 72 additions & 64 deletions src/drafts/SelectPanel2/SelectPanel.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React from 'react'
import {SelectPanel} from './SelectPanel'
import {ActionList, ActionMenu, Avatar, Box, Button} from '../../../src/index'
import {ArrowRightIcon, EyeIcon, GitBranchIcon, TriangleDownIcon} from '@primer/octicons-react'
import {ActionList, ActionMenu, Avatar, Box, Button, Flash} from '../../../src/index'
import {ArrowRightIcon, AlertIcon, EyeIcon, GitBranchIcon, TriangleDownIcon} from '@primer/octicons-react'
import data from './mock-data'

const getCircle = (color: string) => (
Expand Down Expand Up @@ -97,11 +97,12 @@ export const AControlled = () => {
<SelectPanel.Header>
<SelectPanel.SearchInput onChange={onSearchInputChange} />
</SelectPanel.Header>
<SelectPanel.ActionList>
{itemsToShow.length === 0 ? (
<SelectPanel.EmptyMessage>No labels found for &quot;{query}&quot;</SelectPanel.EmptyMessage>
) : (
itemsToShow.map(label => (

{itemsToShow.length === 0 ? (
<SelectPanel.EmptyMessage>No labels found for &quot;{query}&quot;</SelectPanel.EmptyMessage>
) : (
<ActionList>
{itemsToShow.map(label => (
<ActionList.Item
key={label.id}
onSelect={() => onLabelSelect(label.id)}
Expand All @@ -111,9 +112,10 @@ export const AControlled = () => {
{label.name}
<ActionList.Description variant="block">{label.description}</ActionList.Description>
</ActionList.Item>
))
)}
</SelectPanel.ActionList>
))}
</ActionList>
)}

<SelectPanel.Footer>
<SelectPanel.SecondaryButton>Edit labels</SelectPanel.SecondaryButton>
</SelectPanel.Footer>
Expand Down Expand Up @@ -186,24 +188,22 @@ const SuspendedActionList: React.FC<{query: string}> = ({query}) => {

const itemsToShow = query ? filteredLabels : data.labels.sort(sortingFn)

return (
<SelectPanel.ActionList>
{itemsToShow.length === 0 ? (
<SelectPanel.EmptyMessage>No labels found for &quot;{query}&quot;</SelectPanel.EmptyMessage>
) : (
itemsToShow.map(label => (
<ActionList.Item
key={label.id}
onSelect={() => onLabelSelect(label.id)}
selected={selectedLabelIds.includes(label.id)}
>
<ActionList.LeadingVisual>{getCircle(label.color)}</ActionList.LeadingVisual>
{label.name}
<ActionList.Description variant="block">{label.description}</ActionList.Description>
</ActionList.Item>
))
)}
</SelectPanel.ActionList>
return itemsToShow.length === 0 ? (
<SelectPanel.EmptyMessage>No labels found for &quot;{query}&quot;</SelectPanel.EmptyMessage>
) : (
<ActionList>
{itemsToShow.map(label => (
<ActionList.Item
key={label.id}
onSelect={() => onLabelSelect(label.id)}
selected={selectedLabelIds.includes(label.id)}
>
<ActionList.LeadingVisual>{getCircle(label.color)}</ActionList.LeadingVisual>
{label.name}
<ActionList.Description variant="block">{label.description}</ActionList.Description>
</ActionList.Item>
))}
</ActionList>
)
}

Expand Down Expand Up @@ -287,26 +287,24 @@ const SearchableUserList: React.FC<{
}
const itemsToShow = query ? filteredUsers : repository.collaborators.sort(sortingFn)

return (
<SelectPanel.ActionList>
{itemsToShow.length === 0 ? (
<SelectPanel.EmptyMessage>No users found for &quot;{query}&quot;</SelectPanel.EmptyMessage>
) : (
itemsToShow.map(user => (
<ActionList.Item
key={user.id}
onSelect={() => onUserSelect(user.id)}
selected={selectedUserIds.includes(user.id)}
>
<ActionList.LeadingVisual>
<Avatar src={`https://github.com/${user.login}.png`} />
</ActionList.LeadingVisual>
{user.login}
<ActionList.Description>{user.name}</ActionList.Description>
</ActionList.Item>
))
)}
</SelectPanel.ActionList>
return itemsToShow.length === 0 ? (
<SelectPanel.EmptyMessage>No users found for &quot;{query}&quot;</SelectPanel.EmptyMessage>
) : (
<ActionList>
{itemsToShow.map(user => (
<ActionList.Item
key={user.id}
onSelect={() => onUserSelect(user.id)}
selected={selectedUserIds.includes(user.id)}
>
<ActionList.LeadingVisual>
<Avatar src={`https://github.com/${user.login}.png`} />
</ActionList.LeadingVisual>
{user.login}
<ActionList.Description>{user.name}</ActionList.Description>
</ActionList.Item>
))}
</ActionList>
)
}

Expand Down Expand Up @@ -401,15 +399,15 @@ export const TODO1Uncontrolled = () => {
<SelectPanel.SearchInput />
</SelectPanel.Header>

<SelectPanel.ActionList>
<ActionList>
{data.labels.map(label => (
<ActionList.Item key={label.id}>
<ActionList.LeadingVisual>{getCircle(label.color)}</ActionList.LeadingVisual>
{label.name}
<ActionList.Description variant="block">{label.description}</ActionList.Description>
</ActionList.Item>
))}
</SelectPanel.ActionList>
</ActionList>

<SelectPanel.Footer>
<SelectPanel.SecondaryButton>Edit labels</SelectPanel.SecondaryButton>
Expand Down Expand Up @@ -506,11 +504,11 @@ export const HWithFilterButtons = () => {
</Box>
</SelectPanel.Header>

<SelectPanel.ActionList selectionVariant="single">
{itemsToShow.length === 0 ? (
<SelectPanel.EmptyMessage>No labels found for &quot;{'query'}&quot;</SelectPanel.EmptyMessage>
) : (
itemsToShow.map(item => (
{itemsToShow.length === 0 ? (
<SelectPanel.EmptyMessage>No labels found for &quot;{'query'}&quot;</SelectPanel.EmptyMessage>
) : (
<ActionList selectionVariant="single">
{itemsToShow.map(item => (
<ActionList.Item
key={item.id}
selected={selectedRef === item.id}
Expand All @@ -519,9 +517,9 @@ export const HWithFilterButtons = () => {
{item.name}
<ActionList.TrailingVisual>{item.trailingInfo}</ActionList.TrailingVisual>
</ActionList.Item>
))
)}
</SelectPanel.ActionList>
))}
</ActionList>
)}

<SelectPanel.Footer>
<SelectPanel.SecondaryButton as="a" href={`/${selectedFilter}`}>
Expand Down Expand Up @@ -569,7 +567,7 @@ export const EMinimal = () => {
{/* @ts-ignore todo */}
<SelectPanel.Button>Assign label</SelectPanel.Button>

<SelectPanel.ActionList>
<ActionList>
{itemsToShow.map(label => (
<ActionList.Item
key={label.id}
Expand All @@ -581,7 +579,8 @@ export const EMinimal = () => {
<ActionList.Description variant="block">{label.description}</ActionList.Description>
</ActionList.Item>
))}
</SelectPanel.ActionList>
</ActionList>
<SelectPanel.Footer />
</SelectPanel>
</>
)
Expand Down Expand Up @@ -645,7 +644,7 @@ export const FExternalAnchor = () => {
}}
onCancel={() => setOpen(false)} // close on cancel
>
<SelectPanel.ActionList>
<ActionList>
{itemsToShow.map(label => (
<ActionList.Item
key={label.id}
Expand All @@ -657,7 +656,8 @@ export const FExternalAnchor = () => {
<ActionList.Description variant="block">{label.description}</ActionList.Description>
</ActionList.Item>
))}
</SelectPanel.ActionList>
</ActionList>
<SelectPanel.Footer />
</SelectPanel>
</>
)
Expand Down Expand Up @@ -687,6 +687,13 @@ export const GOpenFromMenu = () => {
return (
<>
<h1>Open from ActionMenu</h1>
<Flash variant="danger">
<AlertIcon />
This implementation will most likely change.{' '}
<a href="https://github.com/github/primer/discussions/2614#discussioncomment-6879407">
See decision log for more details.
</a>
</Flash>
<p>
To open SelectPanel from a menu, you would need to use an external anchor and pass `anchorRef` to `SelectPanel`.
You would also need to control the `open` state for both ActionMenu and SelectPanel.
Expand Down Expand Up @@ -761,13 +768,14 @@ export const GOpenFromMenu = () => {
}}
height="medium"
>
<SelectPanel.ActionList>
<ActionList>
{itemsToShow.map(item => (
<ActionList.Item key={item} onSelect={() => onEventSelect(item)} selected={selectedEvents.includes(item)}>
{item}
</ActionList.Item>
))}
</SelectPanel.ActionList>
</ActionList>
<SelectPanel.Footer />
</SelectPanel>
</>
)
Expand Down
65 changes: 37 additions & 28 deletions src/drafts/SelectPanel2/SelectPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@ import {
Tooltip,
TextInput,
AnchoredOverlayProps,
ActionList,
ActionListProps,
Spinner,
Text,
} from '../../../src/index'
import {ActionListContainerContext} from '../../../src/ActionList/ActionListContainerContext'
import {useSlots} from '../../hooks/useSlots'
import {ClearIcon} from './tmp-ClearIcon'
import {useProvidedRefOrCreate} from '../../hooks'
Expand Down Expand Up @@ -55,13 +54,13 @@ const SelectPanel = props => {
React.useEffect(() => setInternalOpen(props.open), [props.open])

const onInternalClose = () => {
if (props.open === 'undefined') setInternalOpen(false)
if (props.open === undefined) setInternalOpen(false)
Copy link
Member

Choose a reason for hiding this comment

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

Oops I didn't catch that in the previous PR too 😬

if (typeof props.onCancel === 'function') props.onCancel()
}
// @ts-ignore todo
const onInternalSubmit = event => {
event.preventDefault()
if (props.open === 'undefined') setInternalOpen(false)
if (props.open === undefined) setInternalOpen(false)
if (typeof props.onSubmit === 'function') props.onSubmit(event)
}

Expand Down Expand Up @@ -100,12 +99,41 @@ const SelectPanel = props => {
setSearchQuery,
}}
>
<Box as="form" onSubmit={onInternalSubmit} sx={{display: 'flex', flexDirection: 'column', height: '100%'}}>
<Box
as="form"
onSubmit={onInternalSubmit}
sx={{
display: 'flex',
flexDirection: 'column',
height: '100%',
}}
>
{/* render default header as fallback */}
{slots.header || <SelectPanel.Header />}
{childrenInBody}
{/* render default footer as fallback */}
{slots.footer || <SelectPanel.Footer />}
<Box
sx={{
Copy link
Member Author

Choose a reason for hiding this comment

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

moved the styles from SelectPanel.ActionList here

flexShrink: 1,
flexGrow: 1,
overflow: 'hidden',

display: 'flex',
flexDirection: 'column',
justifyContent: 'space-between',
ul: {overflowY: 'auto', flexGrow: 1},
}}
>
<ActionListContainerContext.Provider
value={{
container: 'SelectPanel',
listRole: 'listbox',
selectionAttribute: 'aria-selected',
Comment on lines +128 to +129
Copy link
Member

Choose a reason for hiding this comment

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

From what I understand, the role is always going to be listbox and selectionAttribute be aria-selected for SelectPanel. is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that is always true :)

selectionVariant: props.selectionVariant || 'multiple',
}}
>
{childrenInBody}
</ActionListContainerContext.Provider>
</Box>
{slots.footer}
</Box>
</SelectPanelContext.Provider>
</AnchoredOverlay>
Expand Down Expand Up @@ -215,25 +243,6 @@ const SelectPanelSearchInput = props => {
}
SelectPanel.SearchInput = SelectPanelSearchInput

const SelectPanelActionList: React.FC<React.PropsWithChildren<ActionListProps>> = props => {
/* features to implement for uncontrolled:
1. select
2. sort
3. divider
4. search
5. different results view
*/

return (
<>
<ActionList sx={{flexShrink: 1, flexGrow: 1, overflowY: 'auto'}} selectionVariant="multiple" {...props}>
{props.children}
</ActionList>
</>
)
}
SelectPanel.ActionList = SelectPanelActionList

const SelectPanelFooter = ({...props}) => {
const {onCancel} = React.useContext(SelectPanelContext)

Expand Down Expand Up @@ -277,7 +286,7 @@ const SelectPanelLoading: React.FC<{children: string}> = ({children = 'Fetching
flexDirection: 'column',
justifyContent: 'center',
alignItems: 'center',
flexGrow: 1,
height: '100%',
gap: 3,
}}
>
Expand Down
Loading