Skip to content
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

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

Merged
merged 15 commits into from
Oct 18, 2024
Merged
5 changes: 5 additions & 0 deletions .changeset/dull-moons-repair.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

(Behind feature flag) ActionList: Add more guards for `ActionList.Item` before utilizing button semantics
TylerJDev marked this conversation as resolved.
Show resolved Hide resolved
18 changes: 18 additions & 0 deletions packages/react/src/ActionList/ActionList.features.stories.tsx
Original file line number Diff line number Diff 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
Original file line number Diff line number Diff 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
Original file line number Diff line number Diff 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}>
Copy link
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?

Copy link
Contributor Author

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 Down Expand 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']
Copy link
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?

Copy link
Contributor Author

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 Down Expand 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
Loading