Skip to content

feat: implement ADR-023 Part 1 #7724

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

Draft
francinelucca wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

feat: implement ADR-023 Part 1 #7724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
abfe9c4
feat(ActionBar): add data-component attributes for better accessibility
francinelucca Apr 2, 2026
6381280
fix test
francinelucca Apr 2, 2026
091de76
Merge branch 'main' of github.com:primer/react into chore/implement-a…
francinelucca Apr 7, 2026
32d4062
fix(ActionBar): update data-component attributes for IconButton and Menu
francinelucca Apr 7, 2026
c7c0da7
Add data-component for ActionList
francinelucca Apr 8, 2026
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
62 changes: 62 additions & 0 deletions packages/react/src/ActionBar/ActionBar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,3 +417,65 @@ describe('ActionBar.Menu returnFocusRef', () => {
expect(document.activeElement).toEqual(menuButton)
})
})

describe('ActionBar data-component attributes', () => {
it('renders ActionBar with data-component attribute', () => {
const {container} = render(
<ActionBar aria-label="Toolbar">
<ActionBar.IconButton icon={BoldIcon} aria-label="Bold" />
</ActionBar>,
)

const actionBar = container.querySelector('[data-component="ActionBar"]')
expect(actionBar).toBeInTheDocument()
})

it('renders ActionBar.IconButton with data-component attribute', () => {
render(
<ActionBar aria-label="Toolbar">
<ActionBar.IconButton icon={BoldIcon} aria-label="Bold" />
</ActionBar>,
)

const iconButton = screen.getByRole('button', {name: 'Bold'})
expect(iconButton).toHaveAttribute('data-component', 'IconButton')
})

it('renders ActionBar.VerticalDivider with data-component attribute', () => {
const {container} = render(
<ActionBar aria-label="Toolbar">
<ActionBar.IconButton icon={BoldIcon} aria-label="Bold" />
<ActionBar.Divider />
<ActionBar.IconButton icon={ItalicIcon} aria-label="Italic" />
</ActionBar>,
)

const divider = container.querySelector('[data-component="ActionBar.VerticalDivider"]')
expect(divider).toBeInTheDocument()
})

it('renders ActionBar.Group with data-component attribute', () => {
const {container} = render(
<ActionBar aria-label="Toolbar">
<ActionBar.Group>
<ActionBar.IconButton icon={BoldIcon} aria-label="Bold" />
<ActionBar.IconButton icon={ItalicIcon} aria-label="Italic" />
</ActionBar.Group>
</ActionBar>,
)

const group = container.querySelector('[data-component="ActionBar.Group"]')
expect(group).toBeInTheDocument()
})

it('renders ActionBar.Menu with data-component attribute', () => {
render(
<ActionBar aria-label="Toolbar">
<ActionBar.Menu aria-label="More options" icon={BoldIcon} items={[{label: 'Option 1', onClick: vi.fn()}]} />
</ActionBar>,
)

const menuButton = screen.getByRole('button', {name: 'More options'})
expect(menuButton).toHaveAttribute('data-component', 'ActionBar.Menu.IconButton')
})
})
13 changes: 10 additions & 3 deletions packages/react/src/ActionBar/ActionBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ export const ActionBar: React.FC<React.PropsWithChildren<ActionBarProps>> = prop

return (
<ActionBarContext.Provider value={{size, isVisibleChild}}>
<div ref={navRef} className={clsx(className, styles.Nav)} data-flush={flush}>
<div ref={navRef} className={clsx(className, styles.Nav)} data-component="ActionBar" data-flush={flush}>
<div
ref={listRef}
role="toolbar"
Expand Down Expand Up @@ -532,7 +532,7 @@ export const ActionBarGroup = forwardRef(({children}: React.PropsWithChildren, f

return (
<ActionBarGroupContext.Provider value={{groupId: id}}>
<div className={styles.Group} ref={ref}>
<div className={styles.Group} data-component="ActionBar.Group" ref={ref}>
{children}
</div>
</ActionBarGroupContext.Provider>
Expand Down Expand Up @@ -571,7 +571,14 @@ export const ActionBarMenu = forwardRef(
return (
<ActionMenu anchorRef={ref} open={menuOpen} onOpenChange={setMenuOpen}>
<ActionMenu.Anchor>
<IconButton variant="invisible" aria-label={ariaLabel} icon={icon} {...props} />
<IconButton
variant="invisible"
aria-label={ariaLabel}
icon={icon}
{...props}
// overriding IconButton's data-component so that the ActionBar's "More Menu" Icon can be targetted specifically
data-component="ActionBar.Menu.IconButton"
Copy link
Copy Markdown
Member Author

@francinelucca francinelucca Apr 7, 2026

Choose a reason for hiding this comment

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

should this be:

  • ActionBar.MenuIconButton
  • ActionBar.Menu
  • ActionBar.Menu.IconButton
    ?

I feel like this is the IconButton that belongs to the Menu that is a subcomponent of ActionBar, which is why ActionBar.Menu.IconButton makes sense to me 🤔. ActionBar.Menu seems disingenuous because really this is not the menu itself, just the Icon trigger. But I could see the case for ActionBar.MenuIconButton

Copy link
Copy Markdown
Contributor

@llastflowers llastflowers Apr 7, 2026

Choose a reason for hiding this comment

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

I vote for ActionBar.Menu.IconButton too!

Copy link
Copy Markdown
Member

@siddharthkp siddharthkp Apr 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

tl;dr: both ActionBar.MenuIconButton and ActionBar.Menu.IconButton are fine, i like one more than the other

  • ActionBar.Menu 👎 that's for the menu, not the button
  • ActionBar.Menu.IconButton it's fine, but the double dots feels weird, idk why. Maybe because ActionBar.Menu is actually a shorthand for ActionMenu... but ActionBar.ActionMenu.IconButton feels worse for some reason.
  • ActionBar.MenuIconButton: 👍 I like this the most. consistent pattern everywhere

Copy link
Copy Markdown
Member Author

@francinelucca francinelucca Apr 8, 2026

Choose a reason for hiding this comment

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

what about here
image

you vote for ActionList.ItemLabel?

Copy link
Copy Markdown
Member

@siddharthkp siddharthkp Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmmmmm, I know i'm being very inconsistent with my feeling or taste™, but i like ActionList.Item.Label there because ActionList.Item is where you would put your className 🤔

❌ bad:

<ActionList>
  <ActionList.Item><span className={styles.myLabel}>label</span></ActionList.Item>
</ActionList>

<style>
  .myLabel {
    font-weight: bold
  }
</style>

✅ good:

<ActionList>
  <ActionList.Item className={styles.myActionListItem}>label</ActionList.Item>
</ActionList>

<style>
  // with direct data-component, good!:
  .myActionListItem [data-component="ActionList.Item.Label"] {
    font-weight: bold
  }
</style>

I think I was wrong about ActionBar.Menu.IconButton. If ActionBar.Menu is part of the public API, then ActionBar.Menu.IconButton should be the better choice.

Do you mind also including Pagination and SelectPanel in this PR? I think those are the most opaque components, once we go through them, all the rest will be straightforward.

/>
</ActionMenu.Anchor>
<ActionMenu.Overlay {...(returnFocusRef && {returnFocusRef})}>
<ActionList>{items.map((item, index) => renderMenuItem(item, index))}</ActionList>
Expand Down
191 changes: 191 additions & 0 deletions packages/react/src/ActionList/ActionList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,3 +258,194 @@
expect(linkElements[2]).toHaveAttribute('data-size', 'medium') // default should be medium
})
})

describe('ActionList data-component attributes', () => {
it('renders ActionList with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Item>Item</ActionList.Item>
</ActionList>,
)

const actionList = container.querySelector('[data-component="ActionList"]')
expect(actionList).toBeInTheDocument()
})

it('renders ActionList.Item with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Item>Item</ActionList.Item>
</ActionList>,
)

const item = container.querySelector('[data-component="ActionList.Item"]')
expect(item).toBeInTheDocument()
})

it('renders ActionList.Item.Label with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Item>Item</ActionList.Item>
</ActionList>,
)

const label = container.querySelector('[data-component="ActionList.Item.Label"]')
expect(label).toBeInTheDocument()
})

it('renders ActionList.Item--DividerContainer with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Item>Item</ActionList.Item>
</ActionList>,
)

const dividerContainer = container.querySelector('[data-component="ActionList.Item--DividerContainer"]')
expect(dividerContainer).toBeInTheDocument()
})

it('renders ActionList.Group with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Group>
<ActionList.GroupHeading as="h3">Group</ActionList.GroupHeading>
<ActionList.Item>Item</ActionList.Item>
</ActionList.Group>
</ActionList>,
)

const group = container.querySelector('[data-component="ActionList.Group"]')
expect(group).toBeInTheDocument()
})

it('renders ActionList.GroupHeading with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Group>
<ActionList.GroupHeading as="h3">Group</ActionList.GroupHeading>
<ActionList.Item>Item</ActionList.Item>
</ActionList.Group>
</ActionList>,
)

const headingWrap = container.querySelector('[data-component="ActionList.GroupHeading"]')
expect(headingWrap).toBeInTheDocument()
})

it('renders ActionList.GroupHeading with data-component attribute', () => {

Check failure on line 335 in packages/react/src/ActionList/ActionList.test.tsx

View workflow job for this annotation

GitHub Actions / lint 

Test is used multiple times in the same describe(suite) block
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Group>
<ActionList.GroupHeading as="h3">Group Heading</ActionList.GroupHeading>
<ActionList.Item>Item</ActionList.Item>
</ActionList.Group>
</ActionList>,
)

const groupHeading = container.querySelector('[data-component="ActionList.GroupHeading"]')
expect(groupHeading).toBeInTheDocument()
})

it('renders ActionList.Divider with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Item>Item 1</ActionList.Item>
<ActionList.Divider />
<ActionList.Item>Item 2</ActionList.Item>
</ActionList>,
)

const divider = container.querySelector('[data-component="ActionList.Divider"]')
expect(divider).toBeInTheDocument()
})

it('renders ActionList.Description with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Item>
Item
<ActionList.Description>Description</ActionList.Description>
</ActionList.Item>
</ActionList>,
)

const description = container.querySelector('[data-component="ActionList.Description"]')
expect(description).toBeInTheDocument()
})

it('renders ActionList.LeadingVisual with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Item>
<ActionList.LeadingVisual>Icon</ActionList.LeadingVisual>
Item
</ActionList.Item>
</ActionList>,
)

const leadingVisual = container.querySelector('[data-component="ActionList.LeadingVisual"]')
expect(leadingVisual).toBeInTheDocument()
})

it('renders ActionList.TrailingVisual with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Item>
Item
<ActionList.TrailingVisual>Icon</ActionList.TrailingVisual>
</ActionList.Item>
</ActionList>,
)

const trailingVisual = container.querySelector('[data-component="ActionList.TrailingVisual"]')
expect(trailingVisual).toBeInTheDocument()
})

it('renders ActionList.Selection with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList selectionVariant="single" aria-label="List">
<ActionList.Item selected>Item</ActionList.Item>
</ActionList>,
)

const selection = container.querySelector('[data-component="ActionList.Selection"]')
expect(selection).toBeInTheDocument()
})

it('renders ActionList.Heading with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Heading as="h2">Heading</ActionList.Heading>
<ActionList.Item>Item</ActionList.Item>
</ActionList>,
)

const heading = container.querySelector('[data-component="ActionList.Heading"]')
expect(heading).toBeInTheDocument()
})

it('renders ActionList.TrailingAction with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.Item>
Item
<ActionList.TrailingAction label="Action" />
</ActionList.Item>
</ActionList>,
)

const trailingAction = container.querySelector('[data-component="ActionList.TrailingAction"]')
expect(trailingAction).toBeInTheDocument()
})

it('renders ActionList.LinkItem with data-component attribute', () => {
const {container} = HTMLRender(
<ActionList aria-label="List">
<ActionList.LinkItem href="//github.com">Link Item</ActionList.LinkItem>
</ActionList>,
)

const linkItem = container.querySelector('[data-component="ActionList.LinkItem"]')
expect(linkItem).toBeInTheDocument()
})
})
11 changes: 8 additions & 3 deletions packages/react/src/ActionList/Group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,12 @@ export const Group: FCWithSlotMarker<React.PropsWithChildren<ActionListGroupProp
}

return (
<li className={clsx(className, groupClasses.Group)} role={listRole ? 'none' : undefined} {...props}>
<li
className={clsx(className, groupClasses.Group)}
data-component="ActionList.Group"
role={listRole ? 'none' : undefined}
{...props}
>
<GroupContext.Provider value={{selectionVariant, groupHeadingId}}>
{title && !slots.groupHeading ? (
// Escape hatch: supports old API <ActionList.Group title="group title"> in a non breaking way
Expand Down Expand Up @@ -177,7 +182,7 @@ export const GroupHeading: FCWithSlotMarker<React.PropsWithChildren<ActionListGr
className={groupClasses.GroupHeadingWrap}
aria-hidden="true"
data-variant={variant}
data-component="GroupHeadingWrap"
data-component="ActionList.GroupHeading"
as={headingWrapElement}
{...props}
>
Expand All @@ -192,7 +197,7 @@ export const GroupHeading: FCWithSlotMarker<React.PropsWithChildren<ActionListGr
className={groupClasses.GroupHeadingWrap}
data-variant={variant}
as={headingWrapElement}
data-component="GroupHeadingWrap"
data-component="ActionList.GroupHeading"
>
<Heading
className={clsx(className, groupClasses.GroupHeading)}
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/ActionList/Heading.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export const Heading = forwardRef(({as, size, children, visuallyHidden = false,
// use custom id if it is provided. Otherwise, use the id from the context
id={props.id ?? headingId}
className={clsx(className, classes.ActionListHeader)}
data-component="ActionList.Heading"
data-list-variant={listVariant}
{...props}
>
Expand Down
4 changes: 3 additions & 1 deletion packages/react/src/ActionList/Item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ const UnwrappedItem = <As extends React.ElementType = 'li'>(
<li
{...containerProps}
ref={listSemantics ? forwardedRef : null}
data-component="ActionList.Item"
data-variant={variant === 'danger' ? variant : undefined}
data-active={active ? true : undefined}
data-inactive={inactiveText ? true : undefined}
Expand Down Expand Up @@ -347,13 +348,14 @@ const UnwrappedItem = <As extends React.ElementType = 'li'>(
>
{slots.leadingVisual}
</VisualOrIndicator>
{/* TODO: next-major: change to data-component="ActionList.Item.DividerContainer" next major version */}
<span className={classes.ActionListSubContent} data-component="ActionList.Item--DividerContainer">
<ConditionalWrapper
if={!!slots.description}
className={classes.ItemDescriptionWrap}
data-description-variant={descriptionVariant}
>
<span id={labelId} className={classes.ItemLabel}>
<span id={labelId} className={classes.ItemLabel} data-component="ActionList.Item.Label">
{childrenWithoutSlots}
{/* Loading message needs to be in here so it is read with the label */}
{/* If the item is inactive, we do not simultaneously announce that it is loading */}
Expand Down
15 changes: 13 additions & 2 deletions packages/react/src/ActionList/LinkItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,11 @@ const LinkItemComponent = fixedForwardRef(
props.onClick && props.onClick(event as React.MouseEvent<HTMLAnchorElement>)
}
if (inactiveText) {
return <span {...rest}>{children}</span>
return (
<span {...rest} data-component="ActionList.LinkItem">
{children}
</span>
)
}

// Type safety for the polymorphic `as` prop is enforced at the
Expand All @@ -57,7 +61,14 @@ const LinkItemComponent = fixedForwardRef(
// constraint across two polymorphic layers.
const InternalLink: React.ElementType = Link
return (
<InternalLink as={Component} {...rest} {...props} onClick={clickHandler} ref={forwardedRef}>
<InternalLink
as={Component}
{...rest}
{...props}
onClick={clickHandler}
ref={forwardedRef}
data-component="ActionList.LinkItem"
>
{children}
</InternalLink>
)
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/ActionList/List.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ const UnwrappedList = <As extends React.ElementType = 'ul'>(
role={listRole}
aria-labelledby={ariaLabelledBy}
ref={listRef}
data-component="ActionList"
data-dividers={showDividers}
data-variant={variant}
{...restProps}
Expand Down
Loading
Loading