ActionList #1078
ActionList #1078
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/JBY65WCbgQHg37hP6DnvZUA4oqZb |
|
|
|
||
| export function Group({renderItem: _renderItem, ...props}: GroupProps): JSX.Element { | ||
| return <div {...props} /> | ||
| } |
There was a problem hiding this comment.
Just curious, why do we need this component?
src/ActionList/Item.tsx
Outdated
| import styled from 'styled-components' | ||
| import {get} from '../constants' | ||
|
|
||
| interface ItemPropsBase extends React.ComponentPropsWithoutRef<'div'> { |
There was a problem hiding this comment.
I'm going to write some TypeScript authoring guidelines but I think we should avoid including React.ComponentProps<> in the props types that we export. This will make it easier for consumers to use these types with the as prop.
For example, if someone wanted to make a wrapper around ActionList.Item that rendered a button, the div props could make things tricky:
import {ActionList, ActionListItemProps} from '@primer/components'
type MyItemProps = ActionListItemProps & React.ComponentPropsWithoutRef<'button'>
// Now MyItemProps has button and div props :(
function MyItem(props: MyItemProps) {
return <ActionList.Item {...props} as="button" />
}
| interface ItemPropsBase extends React.ComponentPropsWithoutRef<'div'> { | |
| interface ItemPropsBase { |
There was a problem hiding this comment.
I’m definitely open to removing extends React.ComponentPropsWithoutRef<'div'>. 👍
Aside: Re: the as prop, we probably don’t want to allow it for ActionList.Items, since they can contain multiple interactive elements (examples below), but buttons can’t (i.e. nested buttons is not valid HTML).
@vdepizzol and I discussed examples where the ActionList pattern (if not this PR’s implementation) could appear outside DropdownMenu and ActionMenu.
One such example is the issues/PRs list (screenshot below). Items in these lists include titles, labels, and authors; clicking these initiates three different actions. If ActionList.Items were rendered as a button (or an a), the rendered HTML would be invalid.
Another example is the repo file list (screenshot below). Similar to the issue/PR list, each item in the repo file list includes a file name and the most-recent-commit message; clicking these navigates to two different URLs:
…gray' with 'bg.tertiary' and 'border.tertiary'.
…List', 'Header' and 'Item' to vertically and horizontally indent them
…ound. Add padding to align inner text
| {groupId: '3'}, | ||
| {groupId: '4'} | ||
| ]} | ||
| items={[ |
There was a problem hiding this comment.
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"/>
// etcBy 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.
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”)
There was a problem hiding this comment.
(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} />}
]}
/>There was a problem hiding this comment.
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
There was a problem hiding this comment.
Personally I much prefer the data API approach here:
- The list owns grouping, absolving the consumer from this responsibility.
- Limits the ways it can be rendered without an explicit escape hatch (
renderItem) while simultaneously providing a strongly-typed API. - Easier to support things like loading states and extensibility (
itemsas a Promise). - Imagine a
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.
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).
There was a problem hiding this comment.
Closes #1056
Quick Links
Preview
Merge checklist
index.d.ts) if necessaryTake a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.