Skip to content

ActionList: Add more checks for ActionList.Item when using button semantics#4876

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
TylerJDev merged 15 commits into from
Oct 18, 2024
Merged

ActionList: Add more checks for ActionList.Item when using button semantics #4876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
460be86
Add more checks for `ActionList.Item` and button semantics
TylerJDev Aug 23, 2024
88408c5
Turn on FF
TylerJDev Aug 28, 2024
9ff0372
Merge branch 'main' into tylerjdev/action-list-semantic-checks
TylerJDev Sep 12, 2024
89a5e0e
Move events to `li`
TylerJDev Sep 23, 2024
45a8796
Add `role="list"` to role types
TylerJDev Sep 23, 2024
2550b5b
Merge branch 'main' into tylerjdev/action-list-semantic-checks
TylerJDev Sep 23, 2024
b0468d7
Enable flag
TylerJDev Sep 24, 2024
02c62a9
disable the flag
TylerJDev Sep 24, 2024
626ccad
Move `ButtonItemWrapper` outside of `Item`
TylerJDev Sep 27, 2024
9994fa4
Disable flag again
TylerJDev Sep 27, 2024
0750e4a
Remove `itemEventProps`
TylerJDev Sep 30, 2024
9660adc
Enable back FF
TylerJDev Sep 30, 2024
2ead1e4
Add changeset
TylerJDev Sep 30, 2024
6381618
Update .changeset/dull-moons-repair.md
TylerJDev Oct 1, 2024
84aaff8
Merge branch 'main' into tylerjdev/action-list-semantic-checks
TylerJDev Oct 17, 2024
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/dull-moons-repair.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

ActionList: Add more guards for `ActionList.Item` before utilizing button semantics (behind feature flag `primer_react_action_list_item_as_button`)
18 changes: 18 additions & 0 deletions packages/react/src/ActionList/ActionList.features.stories.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -358,6 +358,24 @@ export const ListBoxMultiSelect = () => {
)
}

export const WithDynamicContent = () => {
const [isTrue, setIsTrue] = React.useState(false)

return (
<FeatureFlags flags={{primer_react_action_list_item_as_button: true}}>
<ActionList>
<ActionList.Item
onSelect={() => {
setIsTrue(!isTrue)
}}
>
Activated? {isTrue ? 'Yes' : 'No'}
</ActionList.Item>
</ActionList>
</FeatureFlags>
)
}

export const DisabledSelectedMultiselect = () => (
<ActionList selectionVariant="multiple" role="menu" aria-label="Project">
<ActionList.Item role="menuitemcheckbox" selected aria-checked disabled>
Expand Down
31 changes: 31 additions & 0 deletions packages/react/src/ActionList/ActionList.test.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -586,6 +586,37 @@ describe('ActionList', () => {
expect(mockOnSelect).toHaveBeenCalledTimes(1)
})

it('should not render buttons when feature flag is enabled and is specified role', async () => {
const {getByRole} = HTMLRender(
<FeatureFlags flags={{primer_react_action_list_item_as_button: true}}>
<ActionList>
<ActionList.Item role="option">Item 1</ActionList.Item>
<ActionList.Item role="menuitem">Item 2</ActionList.Item>
<ActionList.Item role="menuitemcheckbox">Item 3</ActionList.Item>
<ActionList.Item role="menuitemradio">Item 4</ActionList.Item>
<ActionList.Item>Item 5</ActionList.Item>
</ActionList>
</FeatureFlags>,
)

const option = getByRole('option')
expect(option.tagName).toBe('LI')
expect(option.textContent).toBe('Item 1')

const menuItem = getByRole('menuitem')
expect(menuItem.tagName).toBe('LI')

const menuItemCheckbox = getByRole('menuitemcheckbox')
expect(menuItemCheckbox.tagName).toBe('LI')

const menuItemRadio = getByRole('menuitemradio')
expect(menuItemRadio.tagName).toBe('LI')

const button = getByRole('button')
expect(button.parentElement?.tagName).toBe('LI')
expect(button.textContent).toBe('Item 5')
})

it('should be navigatable with arrow keys for certain roles', async () => {
HTMLRender(
<ActionList role="listbox" aria-label="Select a project">
Expand Down
30 changes: 15 additions & 15 deletions packages/react/src/ActionList/Item.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -24,6 +24,14 @@ import VisuallyHidden from '../_VisuallyHidden'

const LiBox = styled.li<SxProp>(sx)

const ButtonItemContainer = React.forwardRef(({as: Component = 'button', children, styles, ...props}, forwardedRef) => {
return (
<Box as={Component as React.ElementType} ref={forwardedRef} sx={styles} {...props}>

@siddharthkpsiddharthkpOct 1, 2024

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.

just checking, do we need to merge styles like ButtonItemWrapper or is that not needed anymore?

@TylerJDevTylerJDevOct 1, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I saw we were already handling this on line #329, so now we pass the same logic to ButtonItemContainer, but under the styles prop instead of handling it inside of the component.

{children}
</Box>
)
}) as PolymorphicForwardRefComponent<React.ElementType, ActionListItemProps>

export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
(
{
Expand DownExpand Up@@ -112,7 +120,12 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(

const itemSelectionAttribute = selectionAttribute || inferredSelectionAttribute
// Ensures ActionList.Item retains list item semantics if a valid ARIA role is applied, or if item is inactive
const listSemantics = listRole === 'listbox' || listRole === 'menu' || inactive || container === 'NavList'
const listItemSemantics =
role === 'option' || role === 'menuitem' || role === 'menuitemradio' || role === 'menuitemcheckbox'

const listRoleTypes = ['listbox', 'menu', 'list']

@siddharthkpsiddharthkpOct 1, 2024

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.

Oooh, list is a new addition here. Can you say a bit more about why list was added here and what the remaining cases when we want buttonSemantics to be used?

@TylerJDevTylerJDevOct 1, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

We don't have any live cases of role="list" with ActionList, so this was mainly just an escape hatch. There are odd cases out there where the new semantics might not be as ideal, one I can think of is when ActionList is utilized as a way to load in items (e.g. loading items story). The only issue is, they probably shouldn't be interactive if they're role="list".

Not really beholden to having it here, but thought it could be useful as a escape hatch for components that still need the old semantics, at least temporarily.

const listSemantics =
(listRole && listRoleTypes.includes(listRole)) || inactive || container === 'NavList' || listItemSemantics
const buttonSemantics = !listSemantics && !_PrivateItemWrapper && buttonSemanticsFeatureFlag

const {theme} = useTheme()
Expand DownExpand Up@@ -268,22 +281,9 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
const trailingVisualId = `${itemId}--trailing-visual`
const inactiveWarningId = inactive && !showInactiveIndicator ? `${itemId}--warning-message` : undefined

const ButtonItemWrapper = React.forwardRef(({as: Component = 'button', children, ...props}, forwardedRef) => {
return (
<Box
as={Component as React.ElementType}
sx={merge<BetterSystemStyleObject>(styles, sxProp)}
ref={forwardedRef}
{...props}
>
{children}
</Box>
)
}) as PolymorphicForwardRefComponent<React.ElementType, ActionListItemProps>

let DefaultItemWrapper = React.Fragment
if (buttonSemanticsFeatureFlag) {
DefaultItemWrapper = listSemantics ? React.Fragment : ButtonItemWrapper
DefaultItemWrapper = listSemantics ? React.Fragment : ButtonItemContainer
}

const ItemWrapper = _PrivateItemWrapper || DefaultItemWrapper
Expand Down