Skip to content

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 39 commits into from
Apr 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
da4c45f
feat: Add 'ActionList'
smockle Feb 24, 2021
2b33f5f
feat: Style 'ActionListSectionHeader'
smockle Feb 24, 2021
a3ba467
feat: Increase 'Action List Story' fidelity
smockle Feb 26, 2021
9476cae
chore: Merge branch 'main' into action-list
smockle Mar 4, 2021
445215f
chore: Split ActionList Storybook
smockle Mar 5, 2021
f4112a9
chore: Merge branch 'main' into action-list
smockle Mar 8, 2021
4742e8c
feat: Refactor to accept 'items' and 'groups' and 'renderItems'
smockle Mar 9, 2021
f09cf51
feat: Alternative implementation for 'ActionList'
smockle Mar 9, 2021
8b18e2d
feat: Consolidate 'ActionList.Alternative' and 'ActionList'
smockle Mar 12, 2021
c971a37
chore: Merge branch 'main' into action-list
smockle Mar 17, 2021
90859f0
feat: Refactor to use styled-components rather than 'sx'
smockle Mar 17, 2021
5da511a
chore: Merge branch 'main' into action-list
smockle Mar 18, 2021
0754d97
fix: Replace hardcoded styles with theme values
smockle Mar 19, 2021
d78eb83
tests: Add tests for 'ActionList'
smockle Mar 19, 2021
b95c3ae
docs: Document ActionList
smockle Mar 19, 2021
d92d555
fix: Remove unneeded type guard. Remove IIFE.
smockle Mar 29, 2021
7f321c6
fix: Replace 'forEach' with 'for…of'
smockle Mar 29, 2021
f95e1a0
fix: Use string groupIds instead of number groupIds per t-hugs\' sugg…
smockle Mar 29, 2021
78ed735
fix: Remove 'data-component'
smockle Mar 29, 2021
3397684
fix: Use 'div' instead of 'span' for 'display: flex' leading visual c…
smockle Mar 29, 2021
9aa05fa
fix: Use 'span' instead of 'div' for phrasing content Item description
smockle Mar 29, 2021
cc71ce4
chore: Merge branch 'main' into action-list
smockle Mar 29, 2021
b177ae6
fix: Use 'ThemeProvider' from Primer, not from 'styled-components'
smockle Mar 29, 2021
90fea7a
fix: Remove unused styles
smockle Mar 29, 2021
69a489b
docs: Remove 'render' from 'ActionList' docs example
smockle Mar 30, 2021
0552613
fix: Remove redundant 'theme' prop from 'ThemeProvider' in Storybook …
smockle Mar 30, 2021
55dc1e0
Merge branch 'main' into action-list
smockle Mar 30, 2021
69f1cf0
docs: Explain every 'ActionList' export
smockle Mar 31, 2021
04c0648
fix: Add 'ActionList' example featuring grouped items.
smockle Mar 31, 2021
e602482
chore: Merge branch 'main' into action-list
smockle Mar 31, 2021
1008dce
fix: Adopt functional color variables. Replace 'bg.gray' and 'border.…
smockle Mar 31, 2021
fa98c7c
fix: Run Prettier
smockle Mar 31, 2021
7e98f6b
fix: Hardcode 4px-based space values where it increases readability
smockle Mar 31, 2021
d70271b
fix: Remove negative margin in 'Divider'; instead, apply spacing to '…
smockle Mar 31, 2021
1e44f5c
fix: Remove negative margin in 'filled' 'Header' to left-align backgr…
smockle Mar 31, 2021
b92bc6b
fix: Update 'ActionList' snapshot
smockle Mar 31, 2021
2ca9402
feat: Refactor to support 'renderGroup'
smockle Apr 1, 2021
234b777
fix: Update 'ActionList' snapshot to support 'inset' and 'full' style…
smockle Apr 2, 2021
e065a54
Merge branch 'main' into action-list
smockle Apr 2, 2021
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 .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
"rules": {
"@typescript-eslint/no-explicit-any": 2,
"@typescript-eslint/explicit-module-boundary-types": 0,
"@typescript-eslint/no-unused-vars": ["error", { "argsIgnorePattern": "^_" }]
"@typescript-eslint/no-unused-vars": ["error", {"argsIgnorePattern": "^_"}]
}
}
]
Expand Down
67 changes: 67 additions & 0 deletions docs/content/ActionList.mdx
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={[

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:

<ActionList groups={[
     {groupId: '0'},
     {groupId: '1', header: {title: 'Live query', variant: 'subtle'}},
     {groupId: '2', header: {title: 'Layout', variant: 'subtle'}},
     {groupId: '3'},
     {groupId: '4'}
   ]}>
  <ActionList.Item leadingVisual={TypographyIcon} text="Rename" groupId="0"/>
  // etc

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 😄

Copy link
Member Author

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):

Having read @T-Hugs's doc, I'd be curious to hear if we've tried implementing this component with subcomponents.

With a subcomponent approach, can Items be associated with Groups without resorting to React.Children.map? (cf. “The React.Children anti-pattern”)

Copy link
Member Author

@smockle smockle Apr 2, 2021

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:

<ActionList
  groupMetadata={[
    {groupId: '0'},
    {groupId: '1', header: {title: 'Live query', variant: 'subtle'}},
    {groupId: '2', header: {title: 'Layout', variant: 'subtle'}},
    {groupId: '3'},
    {groupId: '4'}
  ]}
  items={[
    {groupId: '0', renderItem: p => <ActionList.Item leadingVisual={TypographyIcon} text="Rename" {...p} />},
    {groupId: '0', renderItem: p => <ActionList.Item leadingVisual={VersionsIcon} text="Duplicate" {...p} />},
    {groupId: '1', renderItem: p => <ActionList.Item leadingVisual={SearchIcon} text="repo:github/memex,github/github" {...p} />},
    {groupId: '2', renderItem: p => <ActionList.Item leadingVisual={NoteIcon} text="Table" description="Information-dense table optimized for operations across teams" descriptionVariant="block" {...p} />},
    {groupId: '2', renderItem: p => <ActionList.Item leadingVisual={NoteIcon} text="Board" description="Kanban-style board focused on visual states" descriptionVariant="block" {...p} />},
    {groupId: '3', renderItem: p => <ActionList.Item leadingVisual={FilterIcon} text="Save sort and filters to current view" {...p} />},
    {groupId: '3', renderItem: p => <ActionList.Item leadingVisual={FilterIcon} text="Save sort and filters to new view" {...p} />},
    {groupId: '4', renderItem: p => <ActionList.Item leadingVisual={GearIcon} text="View settings" {...p} />}
  ]}
/>

Copy link

@joelhawksley joelhawksley Apr 2, 2021

Choose a reason for hiding this comment

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

With a subcomponent approach, can Items be associated with Groups without resorting to React.Children.map?

I think I might have missed a level of abstraction here! Would this work?

<ActionList />
  <ActionList.Group title="Live query" variant="subtle"} />
    <ActionList.GroupItem leadingVisual={TypographyIcon} text="Rename"/>
   // etc

Copy link
Contributor

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:

  1. The list owns grouping, absolving the consumer from this responsibility.
  2. Limits the ways it can be rendered without an explicit escape hatch (renderItem) while simultaneously providing a strongly-typed API.
  3. Easier to support things like loading states and extensibility (items as a Promise).
  4. Imagine a Dropdown component that uses ActionList. The Dropdown would need a say in the the items rendered by ActionList (e.g. the leading visual to show a checkmark, an onClick handler). This is easy if the Dropdown is passed items as data, which then get modified and forwarded to ActionList. But if your items come as children, you have to do a complicated traversal through the children with React.Children, using cloneElement to effect the right behaviors.

In the interest of tempo, I would like to get this PR merged so we can unblock other components.

Copy link
Member Author

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. 👍

In the interest of tempo, I would like to get this PR merged so we can unblock other components.

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 its React.Children into an items-like array side-by-side with this PR’s items prop. If whether ActionList 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).

Copy link
Member Author

Choose a reason for hiding this comment

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

{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'}
]}
/>
```

## 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. |
25 changes: 25 additions & 0 deletions src/ActionList/Divider.tsx
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
35 changes: 35 additions & 0 deletions src/ActionList/Group.tsx
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>
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why do we need this component?

74 changes: 74 additions & 0 deletions src/ActionList/Header.tsx
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>
)
}
118 changes: 118 additions & 0 deletions src/ActionList/Item.tsx
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;
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>
)
}
Loading