Skip to content

ActionList: Add ActionList.GroupHeading component and update the existing internal Header component for better semantics #3900

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

Merged
broccolinisoup merged 8 commits into from
Nov 10, 2023
Merged

ActionList: Add ActionList.GroupHeading component and update the existing internal Header component for better semantics #3900

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
50340c0
add group heading
broccolinisoup Nov 3, 2023
f8f60f0
update snapshots
broccolinisoup Nov 3, 2023
583b7da
Merge branch 'main' into bs/groupheading-actionlist
broccolinisoup Nov 6, 2023
255360f
add tests, clean up stuff
broccolinisoup Nov 6, 2023
e65f246
update changeset
broccolinisoup Nov 6, 2023
acc4d65
Update src/ActionList/ActionList.test.tsx
broccolinisoup Nov 6, 2023
b929c00
ternary over && and read groupHeadingId from context
broccolinisoup Nov 8, 2023
11daf88
update heading rendering and take the stories back plus add docs
broccolinisoup Nov 10, 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
5 changes: 5 additions & 0 deletions .changeset/giant-radios-sniff.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

ActionList: Add ActionList.GroupHeading component to label the group lists and update labelling semantics for accessibility
140 changes: 140 additions & 0 deletions src/ActionList/ActionList.features.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,146 @@ export const WithVisualListHeading = () => (
</ActionList>
)

export const ListWithNewGroupHeadingAPI = () => (

@joshblack joshblack Nov 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From a documentation perspective, would it help to have the story name be more like ListWithHeading or something similar? Trying to think of when someone might be looking for how to do X (where X is adding a heading for a list) and having a story name that makes sense to them.

The terminology "NewGroupedHeadingAPI" feels very clear for maintainers but from a user perspective might not make as much sense.

@broccolinisoup broccolinisoup Nov 8, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh sorry I forgot to mention that I added these stories for dev/code review only to make the changes easily viewable. Since we are going to deprecate the title prop on ActionList.Group and our new API for providing group title is ActionList.GroupHeading, I was thinking to update the stories that use title prop with ActionList.GroupHeading such as GroupWithFilledTitle and GroupWithSubtleTitle. Would that be helpful and enough you think for consumers to find how to add group headings? Also do you think we should still keep stories with the title prop at all? 🤔

@broccolinisoup broccolinisoup Nov 9, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm going to take these stories back in this PR. I am thinking to do a follow up one that adds a @deprecated notation to the title prop and update all title usages with the new API in stories and docs. Let me know if there is any concern!

<ActionList>
<ActionList.Heading as="h2">List Heading</ActionList.Heading>
<ActionList.Group data-test-id="ActionList.Group">
<ActionList.GroupHeading as="h3">Group 1 Heading</ActionList.GroupHeading>
<ActionList.Item onClick={() => {}}>
<ActionList.LeadingVisual>
<FileDirectoryIcon />
</ActionList.LeadingVisual>
app/assets/modules
</ActionList.Item>
<ActionList.Item onClick={() => {}}>
<ActionList.LeadingVisual>
<FileDirectoryIcon />
</ActionList.LeadingVisual>
src/react/components
</ActionList.Item>
<ActionList.Item onClick={() => {}}>
<ActionList.LeadingVisual>
<FileDirectoryIcon />
</ActionList.LeadingVisual>
memex/shared-ui/components
</ActionList.Item>
<ActionList.Item onClick={() => {}}>
<ActionList.LeadingVisual>
<FileDirectoryIcon />
</ActionList.LeadingVisual>
views/assets/modules
</ActionList.Item>
</ActionList.Group>

<ActionList.Group>
<ActionList.GroupHeading as="h3">Group 2 Heading</ActionList.GroupHeading>
<ActionList.Item onClick={() => {}}>

@joshblack joshblack Nov 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure what our conventions are, but when would we use an empty handler for events versus using storybook actions?

@broccolinisoup broccolinisoup Nov 8, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is a really good question. I am not really sure either. I find storybook actions useful when the story is about the action like the data table action stories you added are great examples for this use case in my opinion. The stories I added or the ones I mentioned here to update are not focusing on actions, they are focusing on semantics, visuals, and having group titles. This is why, I didn't worry about the action itself but I am more than happy to update them to use storybook actions if it is going to be helpful.
Re-thinking about it now, it is probably helpful to log something (storybook actions could be very beneficial in this case) when the click event is trigger at least so empty event handler isn't very helpful here.

What are your thoughts??

<ActionList.LeadingVisual>
<PlusCircleIcon />
</ActionList.LeadingVisual>
Owner
</ActionList.Item>
<ActionList.Item onClick={() => {}}>
<ActionList.LeadingVisual>
<PlusCircleIcon />
</ActionList.LeadingVisual>
Symbol
</ActionList.Item>
<ActionList.Item onClick={() => {}}>
<ActionList.LeadingVisual>
<PlusCircleIcon />
</ActionList.LeadingVisual>
Exclude archived
</ActionList.Item>
</ActionList.Group>
</ActionList>
)

export const MenuWithNewGroupHeadingAPI = () => {
const [assignees, setAssignees] = React.useState(users.slice(0, 1))

const toggleAssignee = (assignee: (typeof users)[number]) => {
const assigneeIndex = assignees.findIndex(a => a.login === assignee.login)

if (assigneeIndex === -1) setAssignees([...assignees, assignee])
else setAssignees(assignees.filter((_, index) => index !== assigneeIndex))
}

return (
<ActionList selectionVariant="multiple" role="menu" showDividers aria-label="Reviewers">
<ActionList.Group>
<ActionList.GroupHeading>Everyone</ActionList.GroupHeading>
{users.slice(2).map(user => (
<ActionList.Item
role="menuitemcheckbox"
key={user.login}
selected={Boolean(assignees.find(assignee => assignee.login === user.login))}
aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))}
onSelect={() => toggleAssignee(user)}
>
<ActionList.LeadingVisual>
<Avatar src={`https://github.com/${user.login}.png`} />
</ActionList.LeadingVisual>
{user.login}
<ActionList.Description>{user.name}</ActionList.Description>
</ActionList.Item>
))}
</ActionList.Group>
</ActionList>
)
}

export const ListboxWithNewGroupHeadingAPI = () => {
const [assignees, setAssignees] = React.useState(users.slice(0, 1))

const toggleAssignee = (assignee: (typeof users)[number]) => {
const assigneeIndex = assignees.findIndex(a => a.login === assignee.login)

if (assigneeIndex === -1) setAssignees([...assignees, assignee])
else setAssignees(assignees.filter((_, index) => index !== assigneeIndex))
}

return (
<ActionList selectionVariant="multiple" role="listbox" showDividers aria-label="Reviewers">
<ActionList.Group>
<ActionList.GroupHeading>Everyone</ActionList.GroupHeading>
{users.slice(2).map(user => (
<ActionList.Item
role="option"
key={user.login}
selected={Boolean(assignees.find(assignee => assignee.login === user.login))}
aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))}
onSelect={() => toggleAssignee(user)}
>
<ActionList.LeadingVisual>
<Avatar src={`https://github.com/${user.login}.png`} />
</ActionList.LeadingVisual>
{user.login}
<ActionList.Description>{user.name}</ActionList.Description>
</ActionList.Item>
))}
</ActionList.Group>
<ActionList.Group>
<ActionList.GroupHeading>Review Requested</ActionList.GroupHeading>
{users.slice(2).map(user => (
<ActionList.Item
role="option"
key={user.login}
selected={Boolean(assignees.find(assignee => assignee.login === user.login))}
aria-checked={Boolean(assignees.find(assignee => assignee.login === user.login))}
onSelect={() => toggleAssignee(user)}
>
<ActionList.LeadingVisual>
<Avatar src={`https://github.com/${user.login}.png`} />
</ActionList.LeadingVisual>
{user.login}
<ActionList.Description>{user.name}</ActionList.Description>
</ActionList.Item>
))}
</ActionList.Group>
</ActionList>
)
}
export const WithCustomHeading = () => (
<>
<Heading as="h1" id="list-heading" sx={{fontSize: 3, marginX: 3}}>
Expand Down
96 changes: 96 additions & 0 deletions src/ActionList/ActionList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,4 +215,100 @@ describe('ActionList', () => {
expect(spy).toHaveBeenCalled()
spy.mockRestore()
})
it('should render the ActionList.GroupHeading component as a heading with the given heading level', async () => {
const container = HTMLRender(
<ActionList>
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Group>
<ActionList.GroupHeading as="h2">Group Heading</ActionList.GroupHeading>
</ActionList.Group>
</ActionList>,
)
const heading = container.getByRole('heading', {level: 2})
expect(heading).toBeInTheDocument()
expect(heading).toHaveTextContent('Group Heading')
})
it('should throw a warning if ActionList.Group is used without as prop when no role is specified (for list role)', async () => {
const spy = jest.spyOn(console, 'warn').mockImplementationOnce(() => {})

HTMLRender(
<ActionList>
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Group>
<ActionList.GroupHeading>Group Heading</ActionList.GroupHeading>
</ActionList.Group>
</ActionList>,
)
expect(spy).toHaveBeenCalledTimes(1)
spy.mockRestore()
})
it('should render the ActionList.GroupHeading component as a span (not a heading tag) when role is specified as listbox', async () => {
const container = HTMLRender(
<ActionList role="listbox">
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Group>
<ActionList.GroupHeading>Group Heading</ActionList.GroupHeading>
</ActionList.Group>
</ActionList>,
)
const label = container.getByText('Group Heading')
expect(label).toBeInTheDocument()
expect(label.tagName).toEqual('SPAN')
})
it('should render the ActionList.GroupHeading component as a span with role="presentation" and aria-hidden="true" when role is specified as listbox', async () => {
const container = HTMLRender(
<ActionList role="listbox">
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Group>
<ActionList.GroupHeading>Group Heading</ActionList.GroupHeading>
</ActionList.Group>
</ActionList>,
)
const label = container.getByText('Group Heading')
const wrapper = label.parentElement
expect(wrapper).toHaveAttribute('role', 'presentation')
expect(wrapper).toHaveAttribute('aria-hidden', 'true')
})
it('should label the list with the group heading id', async () => {
const {container, getByText} = HTMLRender(
<ActionList>
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Group data-test-id="ActionList.Group">
<ActionList.GroupHeading as="h2">Group Heading</ActionList.GroupHeading>
<ActionList.Item>Item</ActionList.Item>
</ActionList.Group>
</ActionList>,
)
const list = container.querySelector(`li[data-test-id='ActionList.Group'] > ul`)
const heading = getByText('Group Heading')
expect(list).toHaveAttribute('aria-labelledby', heading.id)
})
it('should NOT label the list with the group heading id when role is specified', async () => {
const {container, getByText} = HTMLRender(
<ActionList role="listbox">
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Group data-test-id="ActionList.Group">
<ActionList.GroupHeading>Group Heading</ActionList.GroupHeading>
<ActionList.Item>Item</ActionList.Item>
</ActionList.Group>
</ActionList>,
)
const list = container.querySelector(`li[data-test-id='ActionList.Group'] > ul`)
const heading = getByText('Group Heading')
expect(list).not.toHaveAttribute('aria-labelledby', heading.id)
})
it('should label the list using aria-label when role is specified', async () => {
const {container, getByText} = HTMLRender(
<ActionList role="listbox">
<ActionList.Heading as="h1">Heading</ActionList.Heading>
<ActionList.Group data-test-id="ActionList.Group">
<ActionList.GroupHeading>Group Heading</ActionList.GroupHeading>
<ActionList.Item>Item</ActionList.Item>
</ActionList.Group>
</ActionList>,
)
const list = container.querySelector(`li[data-test-id='ActionList.Group'] > ul`)
const heading = getByText('Group Heading')
expect(list).toHaveAttribute('aria-label', heading.textContent)
})
})
94 changes: 79 additions & 15 deletions src/ActionList/Group.tsx
View file
Open in desktop
Comment thread
broccolinisoup marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import React from 'react'
import {useId} from '../hooks/useId'
import Box from '../Box'
import {SxProp} from '../sx'
import {SxProp, BetterSystemStyleObject, merge} from '../sx'
import {ListContext, ActionListProps} from './List'
import {AriaRole} from '../utils/types'
import {default as Heading} from '../Heading'
import type {ActionListHeadingProps} from './Heading'
import {useSlots} from '../hooks/useSlots'
import {defaultSxProp} from '../utils/defaultSxProp'
import {warning} from '../utils/warning'

export type ActionListGroupProps = {
/**
Expand Down Expand Up @@ -44,9 +49,25 @@ export const Group: React.FC<React.PropsWithChildren<ActionListGroupProps>> = ({
sx = {},
...props
}) => {
const labelId = useId()
const id = useId()
const {role: listRole} = React.useContext(ListContext)

const [slots, childrenWithoutSlots] = useSlots(props.children, {
groupHeading: GroupHeading,
})

let groupHeadingId = undefined

// ActionList.GroupHeading
if (slots.groupHeading) {
// If there is an id prop passed in the ActionList.GroupHeading, use it otherwise use the generated id.
groupHeadingId = slots.groupHeading.props.id ?? id
}
// Supports the deprecated `title` prop
if (title) {
groupHeadingId = id
}

return (
<Box
as="li"
Expand All @@ -58,32 +79,64 @@ export const Group: React.FC<React.PropsWithChildren<ActionListGroupProps>> = ({
}}
{...props}
>
{title && <Header title={title} variant={variant} auxiliaryText={auxiliaryText} labelId={labelId} />}
{(slots.groupHeading || title) && (
Comment thread
broccolinisoup marked this conversation as resolved.
Outdated
<GroupHeading
title={title}
variant={variant}
auxiliaryText={auxiliaryText}
groupHeadingId={groupHeadingId}
as={slots.groupHeading?.props.as}
>
{slots.groupHeading ? slots.groupHeading.props.children : null}
</GroupHeading>

@joshblack joshblack Nov 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In this case, are GroupHeading and slots.groupHeading the same component? If so, would cloneElement work in this scenario?

Suggested change
<GroupHeading
title={title}
variant={variant}
auxiliaryText={auxiliaryText}
groupHeadingId={groupHeadingId}
as={slots.groupHeading?.props.as}
>
{slots.groupHeading ? slots.groupHeading.props.children : null}
</GroupHeading>
{cloneElement(slots.groupHeading, {
title,
variant,
auxiliaryText,
groupHeadingId,
})}

@broccolinisoup broccolinisoup Nov 8, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

They are the same components yes.

When I tried cloning though it is complaining about slots.groupHeading might be undefined which is a valid case. If ActionList is using the "to-be-deprecated" API which is <ActionList.Group title="Group heading"> , slots.groupHeading is going to be undefined because I use GroupHeading component to render both (title="Group heading" and <ActionList.GoupHeading>Group heading</GroupHeading>),

They are almost the same component except one is taking title prop and the other is taking children prop to render the heading. I can try splitting them up, or if you have any suggestion I would love to hear it!

)}
<GroupContext.Provider value={{selectionVariant}}>
<Box
as="ul"
sx={{paddingInlineStart: 0}}
aria-labelledby={title ? labelId : undefined}
// if listRole is set (listbox or menu), we don't label the list with the groupHeadingId
// because the heading is hidden from the accessibility tree and only used for presentation role.
// We will instead use aria-label to label the list. See a line below.
aria-labelledby={listRole ? undefined : groupHeadingId}
aria-label={listRole ? title ?? (slots.groupHeading?.props.children as string) : undefined}
role={role || (listRole && 'group')}
>
{props.children}
{slots.groupHeading ? childrenWithoutSlots : props.children}
</Box>
</GroupContext.Provider>
</Box>
)
}

export type HeaderProps = Pick<ActionListGroupProps, 'variant' | 'title' | 'auxiliaryText'> & {
labelId: string
}
export type GroupHeadingProps = Pick<ActionListGroupProps, 'variant' | 'title' | 'auxiliaryText'> &
Omit<ActionListHeadingProps, 'as'> &
SxProp &
React.HTMLAttributes<HTMLElement> & {
as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'
groupHeadingId?: string
Comment thread
joshblack marked this conversation as resolved.
Outdated
}

/**
* Displays the name and description of a `Group`.
*
* For visual presentation only. It's hidden from screen readers.
* For visual presentation only.
*/
const Header: React.FC<React.PropsWithChildren<HeaderProps>> = ({variant, title, auxiliaryText, labelId, ...props}) => {
const {variant: listVariant} = React.useContext(ListContext)
export const GroupHeading: React.FC<React.PropsWithChildren<GroupHeadingProps>> = ({
as,
variant,
title,
auxiliaryText,
groupHeadingId,
children,
sx = defaultSxProp,
...props
}) => {
const {variant: listVariant, role: listRole} = React.useContext(ListContext)
// for list role, the headings are proper heading tags, for menu and listbox, they are just representational and divs
warning(
listRole === undefined && children !== null && as === undefined,
`You are setting a heading for a list, that requires a heading level. Please use 'as' prop to set a proper heading level.`,
)

const styles = {
paddingY: '6px',
Expand All @@ -102,9 +155,20 @@ const Header: React.FC<React.PropsWithChildren<HeaderProps>> = ({variant, title,
}

return (
<Box sx={styles} role="presentation" aria-hidden="true" {...props}>
<span id={labelId}>{title}</span>
{auxiliaryText && <span>{auxiliaryText}</span>}
</Box>
<>
{listRole ? (
<Box sx={styles} role="presentation" aria-hidden="true" {...props}>
<span id={groupHeadingId}>{title ?? children}</span>
{auxiliaryText && <span>{auxiliaryText}</span>}
</Box>
) : (
<>
<Heading as={as || 'h3'} id={groupHeadingId} sx={merge<BetterSystemStyleObject>(styles, sx)} {...props}>
{title ?? children}
</Heading>
{auxiliaryText && <span>{auxiliaryText}</span>}
</>
)}
</>
)
}
Loading