Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 677
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
Uh oh!
There was an error while loading. Please reload this page.
ActionList #1078
Changes from all commits
da4c45f2b33f5fa3ba4679476cae445215ff4112a94742e8cf09cf518b18e2dc971a3790859f05da511a0754d97d78eb83b95c3aed92d5557f321c6f95e1a078ed73533976849aa05facc71ce4b177ae690fea7a69a489b055261355dc1e069f1cf004c0648e6024821008dcefa98c7c7e98f6bd70271b1e44f5cb92bc6b2ca9402234b777e065a54File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| 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.
Uh oh!There was an error while loading. Please reload this page. | ||
| ## 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. | | ||
| 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 |
| 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> | ||
| ) | ||
| } | ||
Contributor 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? | ||
| 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> | ||
| ) | ||
| } |
| 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.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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> | ||
| ) | ||
| } | ||
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.
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
Items be associated withGroups without resorting toReact.Children.map? (cf. “TheReact.Childrenanti-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
ActionListAPI (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.itemsas a Promise).Dropdowncomponent that usesActionList. TheDropdownwould need a say in the the items rendered byActionList(e.g. the leading visual to show a checkmark, anonClickhandler). This is easy if theDropdownis 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, usingcloneElementto 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:
ActionListcould be updated to coalesce itsReact.Childreninto anitems-like array side-by-side with this PR’sitemsprop. If whetherActionListshould 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