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.

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

Jump to bottom

SelectPanel2: Instant selection variant #3783

Merged
merged 8 commits into from
Oct 24, 2023
Merged

SelectPanel2: Instant selection variant #3783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fd7342a
add instant variant example
siddharthkp Oct 2, 2023
5031f91
secondary action only
siddharthkp Oct 2, 2023
3458ab8
multiple is already the default
siddharthkp Oct 19, 2023
a0e163b
Revert "multiple is already the default"
siddharthkp Oct 24, 2023
afa94f6
one step ahead of merge conflict 😇
siddharthkp Oct 24, 2023
7b3aeaa
tmp: remove instant story
siddharthkp Oct 24, 2023
fa04d1a
Merge branch 'main' into drafts-selectpanel-instant
siddharthkp Oct 24, 2023
dfb3eac
bring back instant variant story after merge
siddharthkp Oct 24, 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
35 changes: 34 additions & 1 deletion src/drafts/SelectPanel2/SelectPanel.stories.tsx
View file
Open in desktop
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, Flash} from '../../../src/index'
import {ArrowRightIcon, AlertIcon, EyeIcon, GitBranchIcon, TriangleDownIcon} from '@primer/octicons-react'
import {ArrowRightIcon, AlertIcon, EyeIcon, GitBranchIcon, TriangleDownIcon, TagIcon} from '@primer/octicons-react'
import data from './mock-data'

const getCircle = (color: string) => (
Expand Down Expand Up @@ -839,6 +839,39 @@ export const IWithRemoveFilterIcon = () => {
)
}

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

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

const itemsToShow = data.tags

return (
<>
<h1>Instant selection variant</h1>

<SelectPanel title="Choose a tag" selectionVariant="instant" onSubmit={onSubmit} height="medium" defaultOpen>
{/* @ts-ignore todo */}
<SelectPanel.Button leadingIcon={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.Footer>
<SelectPanel.SecondaryButton>Edit tags</SelectPanel.SecondaryButton>
</SelectPanel.Footer>
</SelectPanel>
</>
)
}

// ----- Suspense implementation details ----

const cache = new Map()
Expand Down
51 changes: 36 additions & 15 deletions src/drafts/SelectPanel2/SelectPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
AnchoredOverlayProps,
Spinner,
Text,
ActionListProps,
} from '../../../src/index'
import {ActionListContainerContext} from '../../../src/ActionList/ActionListContainerContext'
import {useSlots} from '../../hooks/useSlots'
Expand All @@ -24,12 +25,14 @@ const SelectPanelContext = React.createContext<{
onClearSelection: undefined | (() => void)
searchQuery: string
setSearchQuery: () => void
selectionVariant: ActionListProps['selectionVariant'] | 'instant'
}>({
title: '',
onCancel: () => {},
onClearSelection: undefined,
searchQuery: '',
setSearchQuery: () => {},
selectionVariant: 'multiple',
})

// @ts-ignore todo
Expand All @@ -56,9 +59,9 @@ const SelectPanel = props => {
if (props.open === undefined) setInternalOpen(false)
if (typeof props.onCancel === 'function') props.onCancel()
}
// @ts-ignore todo
const onInternalSubmit = event => {
event.preventDefault()

const onInternalSubmit = (event?: React.SyntheticEvent) => {
event?.preventDefault() // there is no event with selectionVariant=instant
if (props.open === undefined) setInternalOpen(false)
if (typeof props.onSubmit === 'function') props.onSubmit(event)
}
Expand All @@ -67,6 +70,10 @@ const SelectPanel = props => {
if (typeof props.onSubmit === 'function') props.onClearSelection()
}

const internalAfterSelect = () => {
if (props.selectionVariant === 'instant') onInternalSubmit()
}

/* Search/Filter */
const [searchQuery, setSearchQuery] = React.useState('')

Expand Down Expand Up @@ -96,6 +103,7 @@ const SelectPanel = props => {
searchQuery,
// @ts-ignore todo
setSearchQuery,
selectionVariant: props.selectionVariant,
}}
>
<Box
Expand Down Expand Up @@ -126,7 +134,9 @@ const SelectPanel = props => {
container: 'SelectPanel',
listRole: 'listbox',
selectionAttribute: 'aria-selected',
selectionVariant: props.selectionVariant || 'multiple',
selectionVariant:
siddharthkp marked this conversation as resolved.
Show resolved Hide resolved
props.selectionVariant === 'instant' ? 'single' : props.selectionVariant || 'multiple',
afterSelect: internalAfterSelect,
}}
>
{childrenInBody}
Expand Down Expand Up @@ -243,7 +253,15 @@ const SelectPanelSearchInput = props => {
SelectPanel.SearchInput = SelectPanelSearchInput

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

const hidePrimaryActions = selectionVariant === 'instant'

if (hidePrimaryActions && !props.children) {
// nothing to render
// todo: we can inform them the developer footer will render nothing
Copy link
Member Author

Choose a reason for hiding this comment

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

not throwing validation errors yet. Will create a PR for all validations errors together

return null
}

return (
<Box
Expand All @@ -255,23 +273,26 @@ const SelectPanelFooter = ({...props}) => {
borderColor: 'border.default',
}}
>
<div>{props.children}</div>
<Box sx={{display: 'flex', gap: 2}}>
<Button size="small" type="button" onClick={() => onCancel()}>
Cancel
</Button>
<Button size="small" type="submit" variant="primary">
Save
</Button>
</Box>
<Box sx={{flexGrow: hidePrimaryActions ? 1 : 0}}>{props.children}</Box>

{hidePrimaryActions ? null : (
<Box sx={{display: 'flex', gap: 2}}>
<Button size="small" type="button" onClick={() => onCancel()}>
Cancel
</Button>
<Button size="small" type="submit" variant="primary">
Save
</Button>
</Box>
)}
Comment on lines +278 to +287
Copy link
Member

Choose a reason for hiding this comment

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

Totally optional - I usually find the syntax below more readable but feel free to ignore if it doesn't resonate with you 🤗

Suggested change
{hidePrimaryActions ? null : (
<Box sx={{display: 'flex', gap: 2}}>
<Button size="small" type="button" onClick={() => onCancel()}>
Cancel
</Button>
<Button size="small" type="submit" variant="primary">
Save
</Button>
</Box>
)}
{!hidePrimaryActions && (
<Box sx={{display: 'flex', gap: 2}}>
<Button size="small" type="button" onClick={() => onCancel()}>
Cancel
</Button>
<Button size="small" type="submit" variant="primary">
Save
</Button>
</Box>
)}

Copy link
Member Author

@siddharthkp siddharthkp Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

but feel free to ignore if it doesn't resonate with you 🤗

Firstly, thank you! I appreciate that!

because I feel the opposite 😅

I find double negatives like !hide confusing. don't hide = so show. There's something really clean about hide → return null

Copy link
Member

Choose a reason for hiding this comment

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

😅 Fair enough!!

</Box>
)
}
SelectPanel.Footer = SelectPanelFooter

// @ts-ignore todo
SelectPanel.SecondaryButton = props => {
return <Button {...props} size="small" type="button" />
return <Button {...props} size="small" type="button" block />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering why this Button needs to be block type?

Copy link
Member Author

@siddharthkp siddharthkp Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good question!

So, for most cases of SelectPanel you'd see a Save and Cancel button on the right, but then also an optional secondary button on the left:

select panel with save, cancel and a secondary action in the footer

In the case of instant variant, there is no Save and Cancel. So, we'd like the secondary button to take the full width of the footer so it looks less awkward:

Without block With block
select panel with just a secondary action in the footer, taking the full width of the footer select panel with just a secondary action in the footer, awkwardly sitting in the left corner

Now because the footer has 2 divs inside of it - left buttons container and right buttons container, it's the left container that "flex-grows" based on variant. Inside the left container, secondary button can always take the full width (block) available to it.

left container with flexGrow: 0 left container with flexGrow: 1
image image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see! thanks for the explainer!

}
// SelectPanel.SecondaryLink = props => {
// return <a {...props}>{props.children}</a>
Expand Down
4 changes: 3 additions & 1 deletion src/drafts/SelectPanel2/mock-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,7 +922,9 @@ const data = {
{id: 'dependabot/markdownlint-github-0.4.1', name: 'dependabot/markdownlint-github-0.4.1'},
],
tags: [
{id: 'v35.29.0', name: 'v35.29.0', trailingInfo: 'Latest'},
{id: 'v35.31.0', name: 'v35.31.0', trailingInfo: 'Latest'},
{id: 'v35.30.0', name: 'v35.30.0'},
{id: 'v35.29.0', name: 'v35.29.0'},
{id: 'v35.28.0', name: 'v35.28.0'},
{id: 'v35.27.1', name: 'v35.27.1'},
{id: 'v35.27.0', name: 'v35.27.0'},
Expand Down
Loading