-
Notifications
You must be signed in to change notification settings - Fork 616
ActionList #1078
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
Merged
Merged
ActionList #1078
Changes from all commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
da4c45f
feat: Add 'ActionList'
smockle 2b33f5f
feat: Style 'ActionListSectionHeader'
smockle a3ba467
feat: Increase 'Action List Story' fidelity
smockle 9476cae
chore: Merge branch 'main' into action-list
smockle 445215f
chore: Split ActionList Storybook
smockle f4112a9
chore: Merge branch 'main' into action-list
smockle 4742e8c
feat: Refactor to accept 'items' and 'groups' and 'renderItems'
smockle f09cf51
feat: Alternative implementation for 'ActionList'
smockle 8b18e2d
feat: Consolidate 'ActionList.Alternative' and 'ActionList'
smockle c971a37
chore: Merge branch 'main' into action-list
smockle 90859f0
feat: Refactor to use styled-components rather than 'sx'
smockle 5da511a
chore: Merge branch 'main' into action-list
smockle 0754d97
fix: Replace hardcoded styles with theme values
smockle d78eb83
tests: Add tests for 'ActionList'
smockle b95c3ae
docs: Document ActionList
smockle d92d555
fix: Remove unneeded type guard. Remove IIFE.
smockle 7f321c6
fix: Replace 'forEach' with 'for…of'
smockle f95e1a0
fix: Use string groupIds instead of number groupIds per t-hugs\' sugg…
smockle 78ed735
fix: Remove 'data-component'
smockle 3397684
fix: Use 'div' instead of 'span' for 'display: flex' leading visual c…
smockle 9aa05fa
fix: Use 'span' instead of 'div' for phrasing content Item description
smockle cc71ce4
chore: Merge branch 'main' into action-list
smockle b177ae6
fix: Use 'ThemeProvider' from Primer, not from 'styled-components'
smockle 90fea7a
fix: Remove unused styles
smockle 69a489b
docs: Remove 'render' from 'ActionList' docs example
smockle 0552613
fix: Remove redundant 'theme' prop from 'ThemeProvider' in Storybook …
smockle 55dc1e0
Merge branch 'main' into action-list
smockle 69f1cf0
docs: Explain every 'ActionList' export
smockle 04c0648
fix: Add 'ActionList' example featuring grouped items.
smockle e602482
chore: Merge branch 'main' into action-list
smockle 1008dce
fix: Adopt functional color variables. Replace 'bg.gray' and 'border.…
smockle fa98c7c
fix: Run Prettier
smockle 7e98f6b
fix: Hardcode 4px-based space values where it increases readability
smockle d70271b
fix: Remove negative margin in 'Divider'; instead, apply spacing to '…
smockle 1e44f5c
fix: Remove negative margin in 'filled' 'Header' to left-align backgr…
smockle b92bc6b
fix: Update 'ActionList' snapshot
smockle 2ca9402
feat: Refactor to support 'renderGroup'
smockle 234b777
fix: Update 'ActionList' snapshot to support 'inset' and 'full' style…
smockle e065a54
Merge branch 'main' into action-list
smockle File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
--- | ||
title: ActionList | ||
--- | ||
|
||
An `ActionList` is a list of items which can be activated or selected. `ActionList` is the base component for many of our menu-type components, including `DropdownMenu` and `ActionMenu`. | ||
|
||
## Minimal example | ||
|
||
```jsx live | ||
<ActionList | ||
items={[ | ||
{text: 'New file'}, | ||
ActionList.Divider, | ||
{text: 'Copy link'}, | ||
{text: 'Edit file'}, | ||
{text: 'Delete file', variant: 'danger'} | ||
]} | ||
/> | ||
``` | ||
|
||
## Example with grouped items | ||
|
||
```jsx live | ||
<ActionList | ||
groupMetadata={[ | ||
{groupId: '0'}, | ||
{groupId: '1', header: {title: 'Live query', variant: 'subtle'}}, | ||
{groupId: '2', header: {title: 'Layout', variant: 'subtle'}}, | ||
{groupId: '3'}, | ||
{groupId: '4'} | ||
]} | ||
items={[ | ||
{leadingVisual: TypographyIcon, text: 'Rename', groupId: '0'}, | ||
{leadingVisual: VersionsIcon, text: 'Duplicate', groupId: '0'}, | ||
{leadingVisual: SearchIcon, text: 'repo:github/memex,github/github', groupId: '1'}, | ||
{ | ||
leadingVisual: NoteIcon, | ||
text: 'Table', | ||
description: 'Information-dense table optimized for operations across teams', | ||
descriptionVariant: 'block', | ||
groupId: '2' | ||
}, | ||
{ | ||
leadingVisual: ProjectIcon, | ||
text: 'Board', | ||
description: 'Kanban-style board focused on visual states', | ||
descriptionVariant: 'block', | ||
groupId: '2' | ||
}, | ||
{ | ||
leadingVisual: FilterIcon, | ||
text: 'Save sort and filters to current view', | ||
groupId: '3' | ||
}, | ||
{leadingVisual: FilterIcon, text: 'Save sort and filters to new view', groupId: '3'}, | ||
{leadingVisual: GearIcon, text: 'View settings', groupId: '4'} | ||
]} | ||
/> | ||
``` | ||
|
||
smockle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
## Component props | ||
|
||
| Name | Type | Default | Description | | ||
| :------------ | :---------------------------------- | :---------------: | :------------------------------------------------------------------------------------------------------------------------------------------------------ | | ||
| items | `ItemProps[]` | `undefined` | Required. A list of item objects conforming to the `ActionList.Item` props interface. | | ||
| renderItems | `(props: ItemProps) => JSX.Element` | `ActionList.Item` | Optional. If defined, each item in `items` will be passed to this function, allowing for `ActionList`-wide custom item rendering. | | ||
| groupMetadata | `GroupProps[]` | `undefined` | Optional. If defined, `ActionList` will group `items` into `ActionList.Group`s separated by `ActionList.Divider` according to their `groupId` property. | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import React from 'react' | ||
import styled from 'styled-components' | ||
import {get} from '../constants' | ||
|
||
const StyledDivider = styled.div` | ||
height: 1px; | ||
background: ${get('colors.selectMenu.borderSecondary')}; | ||
margin-top: calc(${get('space.2')} - 1px); | ||
margin-bottom: ${get('space.2')}; | ||
` | ||
|
||
/** | ||
* Visually separates `Item`s or `Group`s in an `ActionList`. | ||
*/ | ||
export function Divider(): JSX.Element { | ||
return <StyledDivider /> | ||
} | ||
|
||
/** | ||
* `Divider` fulfills the `ItemPropsWithCustomRenderer` contract, | ||
* so it can be used inline in an `ActionList`’s `items` prop. | ||
* In other words, `items={[ActionList.Divider]}` is supported as a concise | ||
* alternative to `items={[{renderItem: () => <ActionList.Divider />}]}`. | ||
*/ | ||
Divider.renderItem = Divider |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import React from 'react' | ||
import styled from 'styled-components' | ||
import sx, {SxProp} from '../sx' | ||
import {Header, HeaderProps} from './Header' | ||
|
||
/** | ||
* Contract for props passed to the `Group` component. | ||
*/ | ||
export interface GroupProps extends React.ComponentPropsWithoutRef<'div'>, SxProp { | ||
/** | ||
* Props for a `Header` to render in the `Group`. | ||
*/ | ||
header?: HeaderProps | ||
|
||
/** | ||
* `Items` to render in the `Group`. | ||
*/ | ||
items?: JSX.Element[] | ||
} | ||
|
||
const StyledGroup = styled.div` | ||
${sx} | ||
` | ||
|
||
/** | ||
* Collects related `Items` in an `ActionList`. | ||
*/ | ||
export function Group({header, items, ...props}: GroupProps): JSX.Element { | ||
return ( | ||
<StyledGroup {...props}> | ||
{header && <Header {...header} />} | ||
{items} | ||
</StyledGroup> | ||
) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious, why do we need this component? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
import React from 'react' | ||
import styled, {css} from 'styled-components' | ||
import {get} from '../constants' | ||
import sx, {SxProp} from '../sx' | ||
|
||
/** | ||
* Contract for props passed to the `Header` component. | ||
*/ | ||
export interface HeaderProps extends React.ComponentPropsWithoutRef<'div'>, SxProp { | ||
/** | ||
* Style variations. Usage is discretionary. | ||
* | ||
* - `"filled"` - Superimposed on a background, offset from nearby content | ||
* - `"subtle"` - Relatively less offset from nearby content | ||
*/ | ||
variant?: 'subtle' | 'filled' | ||
|
||
/** | ||
* Primary text which names a `Group`. | ||
*/ | ||
title: string | ||
|
||
/** | ||
* Secondary text which provides additional information about a `Group`. | ||
*/ | ||
auxiliaryText?: string | ||
} | ||
|
||
const StyledHeader = styled.div<{variant: HeaderProps['variant']} & SxProp>` | ||
{ | ||
/* 6px vertical padding + 20px line height = 32px total height | ||
* | ||
* TODO: When rem-based spacing on a 4px scale lands, replace | ||
* hardcoded '6px' with 'calc((${get('space.s32')} - ${get('space.20')}) / 2)'. | ||
*/ | ||
} | ||
padding: 6px ${get('space.3')}; | ||
font-size: ${get('fontSizes.0')}; | ||
font-weight: ${get('fontWeights.bold')}; | ||
color: ${get('colors.text.secondary')}; | ||
|
||
${({variant}) => | ||
variant === 'filled' && | ||
css` | ||
background: ${get('colors.bg.tertiary')}; | ||
margin: ${get('space.2')} 0; | ||
border-top: 1px solid ${get('colors.border.tertiary')}; | ||
border-bottom: 1px solid ${get('colors.border.tertiary')}; | ||
|
||
&:first-child { | ||
margin-top: 0; | ||
} | ||
`} | ||
|
||
${sx} | ||
` | ||
|
||
/** | ||
* Displays the name and description of a `Group`. | ||
*/ | ||
export function Header({ | ||
variant = 'subtle', | ||
title, | ||
auxiliaryText, | ||
children: _children, | ||
...props | ||
}: HeaderProps): JSX.Element { | ||
return ( | ||
<StyledHeader role="heading" variant={variant} {...props}> | ||
{title} | ||
{auxiliaryText && <span>auxiliaryText</span>} | ||
</StyledHeader> | ||
) | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
import type {IconProps} from '@primer/octicons-react' | ||
import React from 'react' | ||
import styled from 'styled-components' | ||
import {get} from '../constants' | ||
import sx, {SxProp} from '../sx' | ||
|
||
/** | ||
* Contract for props passed to the `Item` component. | ||
*/ | ||
export interface ItemProps extends React.ComponentPropsWithoutRef<'div'>, SxProp { | ||
/** | ||
* Primary text which names an `Item`. | ||
*/ | ||
text: string | ||
|
||
/** | ||
* Secondary text which provides additional information about an `Item`. | ||
*/ | ||
description?: string | ||
|
||
/** | ||
* Secondary text style variations. Usage is discretionary. | ||
* | ||
* - `"inline"` - Secondary text is positioned beside primary text. | ||
* - `"block"` - Secondary text is positioned below primary text. | ||
*/ | ||
descriptionVariant?: 'inline' | 'block' | ||
|
||
/** | ||
* Icon (or similar) positioned before `Item` text. | ||
*/ | ||
leadingVisual?: React.FunctionComponent<IconProps> | ||
|
||
/** | ||
* Style variations associated with various `Item` types. | ||
* | ||
* - `"default"` - An action `Item`. | ||
* - `"danger"` - A destructive action `Item`. | ||
*/ | ||
variant?: 'default' | 'danger' | ||
} | ||
|
||
const StyledItem = styled.div<{variant: ItemProps['variant']} & SxProp>` | ||
/* 6px vertical padding + 20px line height = 32px total height | ||
* | ||
* TODO: When rem-based spacing on a 4px scale lands, replace | ||
* hardcoded '6px' with 'calc((${get('space.s32')} - ${get('space.20')}) / 2)'. | ||
*/ | ||
padding: 6px ${get('space.2')}; | ||
display: flex; | ||
border-radius: ${get('radii.2')}; | ||
color: ${({variant}) => (variant === 'danger' ? get('colors.text.danger') : 'inherit')}; | ||
|
||
@media (hover: hover) and (pointer: fine) { | ||
:hover { | ||
background: ${props => | ||
props.variant === 'danger' ? get('colors.bg.danger') : get('colors.selectMenu.tapHighlight')}; | ||
cursor: pointer; | ||
} | ||
} | ||
|
||
${sx} | ||
` | ||
|
||
const StyledTextContainer = styled.div<{descriptionVariant: ItemProps['descriptionVariant']}>` | ||
flex-direction: ${({descriptionVariant}) => (descriptionVariant === 'inline' ? 'row' : 'column')}; | ||
` | ||
|
||
const LeadingVisualContainer = styled.div` | ||
{ | ||
/* Match visual height to adjacent text line height. | ||
* | ||
* TODO: When rem-based spacing on a 4px scale lands, replace | ||
* hardcoded '20px' with '${get('space.s20')}'. | ||
*/ | ||
} | ||
height: 20px; | ||
width: ${get('space.3')}; | ||
display: flex; | ||
smockle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
flex-direction: column; | ||
justify-content: center; | ||
margin-right: ${get('space.2')}; | ||
|
||
svg { | ||
fill: ${get('colors.text.secondary')}; | ||
font-size: ${get('fontSizes.0')}; | ||
} | ||
` | ||
|
||
const DescriptionContainer = styled.span` | ||
color: ${get('colors.text.secondary')}; | ||
` | ||
|
||
/** | ||
* An actionable or selectable `Item` with an optional icon and description. | ||
*/ | ||
export function Item({ | ||
text, | ||
description, | ||
descriptionVariant = 'inline', | ||
leadingVisual: LeadingVisual, | ||
variant = 'default', | ||
...props | ||
}: Partial<ItemProps>): JSX.Element { | ||
return ( | ||
<StyledItem variant={variant} {...props}> | ||
{LeadingVisual && ( | ||
<LeadingVisualContainer> | ||
<LeadingVisual /> | ||
</LeadingVisualContainer> | ||
)} | ||
<StyledTextContainer descriptionVariant={descriptionVariant}> | ||
<div>{text}</div> | ||
{description && <DescriptionContainer>{description}</DescriptionContainer>} | ||
</StyledTextContainer> | ||
</StyledItem> | ||
) | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having read @T-Hugs's doc, I'd be curious to hear if we've tried implementing this component with subcomponents.
I recognize that I don't have a ton of context here, and that most of my relevant experience is on the Rails side, but several existing Primer React components use subcomponents, such as TabNav, and I'm wondering if it might be a better long-term option here.
The reason I ask is that it feels like we are passing a lot of information to ActionList about its children, effectively having it conceptually cover two different levels of hierarchy (parent and children). While it is not onerous now, I can imagine a future where we need to double the number of options for child items, leading to a significantly larger API for this single component.
Instead, I could see us having:
By splitting the API across the two levels of hierarchy, we get to divide the number of properties between the parent and children. Having seen the breadth and depth of @vdepizzol's ideas for overlays, I expect there to be expansion in both dimensions in the future 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From @joelhawksley in #1078 (comment):
With a subcomponent approach, can
Item
s be associated withGroup
s without resorting toReact.Children.map
? (cf. “TheReact.Children
anti-pattern”)Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(You’re asking about coupling between parent and child types—“it feels like we are passing a lot of information to ActionList about its children, effectively having it conceptually cover two different levels of hierarchy (parent and children)”—but as an aside:) If syntax was also a concern, the
ActionList
API (as implemented in this PR) supports JSX in addition to JSON:Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I might have missed a level of abstraction here! Would this work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I much prefer the data API approach here:
renderItem
) while simultaneously providing a strongly-typed API.items
as a Promise).Dropdown
component that usesActionList
. TheDropdown
would need a say in the the items rendered byActionList
(e.g. the leading visual to show a checkmark, anonClick
handler). This is easy if theDropdown
is passed items as data, which then get modified and forwarded toActionList
. But if your items come as children, you have to do a complicated traversal through the children withReact.Children
, usingcloneElement
to effect the right behaviors.In the interest of tempo, I would like to get this PR merged so we can unblock other components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for enumerating some of the benefits of an items-as-data approach, @T-Hugs. 👍
Sounds good 👍 I’m merging this PR, because doing so doesn’t preclude us from supporting other component APIs in the future:
ActionList
could be updated to coalesce itsReact.Children
into anitems
-like array side-by-side with this PR’sitems
prop. If whetherActionList
should do so is a topic of continued interest, merging is non-disruptive.Merging unblocks #1152 and #1135 and allows us to ship-to-learn about the ergonomics and DX of this component API outside of contrived Storybook stories, e.g. in situations where the list of items is not available when the component is initially loaded (as anticipated by @T-Hugs in “3.” above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: https://github.com/github/primer/discussions/143