From 460be861d3a538a095e4cb0578a3e2c1a6d2496e Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Fri, 23 Aug 2024 15:24:04 -0400 Subject: [PATCH 01/12] Add more checks for `ActionList.Item` and button semantics --- .../react/src/ActionList/ActionList.test.tsx | 31 +++++++++++++++++++ packages/react/src/ActionList/Item.tsx | 5 ++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/react/src/ActionList/ActionList.test.tsx b/packages/react/src/ActionList/ActionList.test.tsx index 04a51881d26..892d68f0c47 100644 --- a/packages/react/src/ActionList/ActionList.test.tsx +++ b/packages/react/src/ActionList/ActionList.test.tsx @@ -584,6 +584,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( + + + Item 1 + Item 2 + Item 3 + Item 4 + Item 5 + + , + ) + + 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( diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index a8884739d99..df7e53ad536 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -112,7 +112,10 @@ export const Item = React.forwardRef( 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 listSemantics = + listRole === 'listbox' || listRole === 'menu' || inactive || container === 'NavList' || listItemSemantics const buttonSemantics = !listSemantics && !_PrivateItemWrapper && buttonSemanticsFeatureFlag const {theme} = useTheme() From 88408c5aa7adce07b6f9b37473f47c79d4c37ae9 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Wed, 28 Aug 2024 09:59:19 -0400 Subject: [PATCH 02/12] Turn on FF --- packages/react/src/ActionList/Item.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index df7e53ad536..9d0b6982c00 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -53,7 +53,7 @@ export const Item = React.forwardRef( const {container, afterSelect, selectionAttribute, defaultTrailingVisual} = React.useContext(ActionListContainerContext) - const buttonSemanticsFeatureFlag = useFeatureFlag('primer_react_action_list_item_as_button') + const buttonSemanticsFeatureFlag = true // useFeatureFlag('primer_react_action_list_item_as_button') // Be sure to avoid rendering the container unless there is a default const wrappedDefaultTrailingVisual = defaultTrailingVisual ? ( From 89a5e0e7550766261b94366b71c5896cd61b6597 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Mon, 23 Sep 2024 11:09:29 -0400 Subject: [PATCH 03/12] Move events to `li` --- packages/react/src/ActionList/Item.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index 6dc7e923bb1..b318d45d4bf 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -295,9 +295,12 @@ export const Item = React.forwardRef( const selectableRoles = ['menuitemradio', 'menuitemcheckbox', 'option'] const includeSelectionAttribute = itemSelectionAttribute && itemRole && selectableRoles.includes(itemRole) - const menuItemProps = { + const itemEventProps = { onClick: clickHandler, onKeyPress: !buttonSemantics ? keyPressHandler : undefined, + } + + const menuItemProps = { 'aria-disabled': disabled ? true : undefined, 'data-inactive': inactive ? true : undefined, 'data-loading': loading && !inactive ? true : undefined, @@ -311,6 +314,7 @@ export const Item = React.forwardRef( .join(' ') .trim() || undefined, ...(includeSelectionAttribute && {[itemSelectionAttribute]: selected}), + ...(buttonSemanticsFeatureFlag ? {} : itemEventProps), role: itemRole, id: itemId, } @@ -322,10 +326,10 @@ export const Item = React.forwardRef( containerProps = _PrivateItemWrapper ? {role: itemRole ? 'none' : undefined, ...props} : // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - (listSemantics && {...menuItemProps, ...props, ref: forwardedRef}) || {} + (listSemantics && {...menuItemProps, ...itemEventProps, ...props, ref: forwardedRef}) || {...itemEventProps} wrapperProps = _PrivateItemWrapper - ? menuItemProps + ? {...menuItemProps, ...itemEventProps} : !listSemantics && { ...menuItemProps, ...props, From 45a879673dae142fa80254906f111ae5d0107da8 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Mon, 23 Sep 2024 16:06:21 -0400 Subject: [PATCH 04/12] Add `role="list"` to role types --- packages/react/src/ActionList/Item.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index b318d45d4bf..2a0540f817c 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -114,8 +114,10 @@ export const Item = React.forwardRef( // Ensures ActionList.Item retains list item semantics if a valid ARIA role is applied, or if item is inactive const listItemSemantics = role === 'option' || role === 'menuitem' || role === 'menuitemradio' || role === 'menuitemcheckbox' + + const listRoleTypes = ['listbox', 'menu', 'list'] const listSemantics = - listRole === 'listbox' || listRole === 'menu' || inactive || container === 'NavList' || listItemSemantics + (listRole && listRoleTypes.includes(listRole)) || inactive || container === 'NavList' || listItemSemantics const buttonSemantics = !listSemantics && !_PrivateItemWrapper && buttonSemanticsFeatureFlag const {theme} = useTheme() From b0468d78f0641cdf7421c48aaf5371380146b958 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Tue, 24 Sep 2024 10:16:02 -0400 Subject: [PATCH 05/12] Enable flag --- packages/react/src/ActionList/Item.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index 072c96546fb..dcc27d49600 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -53,7 +53,7 @@ export const Item = React.forwardRef( const {container, afterSelect, selectionAttribute, defaultTrailingVisual} = React.useContext(ActionListContainerContext) - const buttonSemanticsFeatureFlag = true // useFeatureFlag('primer_react_action_list_item_as_button') + const buttonSemanticsFeatureFlag = useFeatureFlag('primer_react_action_list_item_as_button') // Be sure to avoid rendering the container unless there is a default const wrappedDefaultTrailingVisual = defaultTrailingVisual ? ( From 02c62a9501a06712b0a4b85e01f4fcd78358fe57 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Tue, 24 Sep 2024 10:55:16 -0400 Subject: [PATCH 06/12] disable the flag --- packages/react/src/ActionList/Item.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index dcc27d49600..072c96546fb 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -53,7 +53,7 @@ export const Item = React.forwardRef( const {container, afterSelect, selectionAttribute, defaultTrailingVisual} = React.useContext(ActionListContainerContext) - const buttonSemanticsFeatureFlag = useFeatureFlag('primer_react_action_list_item_as_button') + const buttonSemanticsFeatureFlag = true // useFeatureFlag('primer_react_action_list_item_as_button') // Be sure to avoid rendering the container unless there is a default const wrappedDefaultTrailingVisual = defaultTrailingVisual ? ( From 626ccad5835b176af6b6b981102b2adc311e26ec Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Fri, 27 Sep 2024 19:33:12 -0400 Subject: [PATCH 07/12] Move `ButtonItemWrapper` outside of `Item` --- .../ActionList.features.stories.tsx | 18 +++++++++++++++ packages/react/src/ActionList/Item.tsx | 23 ++++++++----------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/packages/react/src/ActionList/ActionList.features.stories.tsx b/packages/react/src/ActionList/ActionList.features.stories.tsx index c7e6c6be821..fcb2742232b 100644 --- a/packages/react/src/ActionList/ActionList.features.stories.tsx +++ b/packages/react/src/ActionList/ActionList.features.stories.tsx @@ -358,6 +358,24 @@ export const ListBoxMultiSelect = () => { ) } +export const WithDynamicContent = () => { + const [isTrue, setIsTrue] = React.useState(false) + + return ( + + + { + setIsTrue(!isTrue) + }} + > + Activated? {isTrue ? 'Yes' : 'No'} + + + + ) +} + export const DisabledSelectedMultiselect = () => ( diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index 072c96546fb..fc1be1a7caa 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -24,6 +24,14 @@ import VisuallyHidden from '../_VisuallyHidden' const LiBox = styled.li(sx) +const ButtonItemWrapper = React.forwardRef(({as: Component = 'button', children, styles, ...props}, forwardedRef) => { + return ( + + {children} + + ) +}) as PolymorphicForwardRefComponent + export const Item = React.forwardRef( ( { @@ -53,7 +61,7 @@ export const Item = React.forwardRef( const {container, afterSelect, selectionAttribute, defaultTrailingVisual} = React.useContext(ActionListContainerContext) - const buttonSemanticsFeatureFlag = true // useFeatureFlag('primer_react_action_list_item_as_button') + const buttonSemanticsFeatureFlag = useFeatureFlag('primer_react_action_list_item_as_button') // Be sure to avoid rendering the container unless there is a default const wrappedDefaultTrailingVisual = defaultTrailingVisual ? ( @@ -273,19 +281,6 @@ export const Item = React.forwardRef( const trailingVisualId = `${itemId}--trailing-visual` const inactiveWarningId = inactive && !showInactiveIndicator ? `${itemId}--warning-message` : undefined - const ButtonItemWrapper = React.forwardRef(({as: Component = 'button', children, ...props}, forwardedRef) => { - return ( - (styles, sxProp)} - ref={forwardedRef} - {...props} - > - {children} - - ) - }) as PolymorphicForwardRefComponent - let DefaultItemWrapper = React.Fragment if (buttonSemanticsFeatureFlag) { DefaultItemWrapper = listSemantics ? React.Fragment : ButtonItemWrapper From 9994fa49fc421448669cfb421c82bc7afc976a8a Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Fri, 27 Sep 2024 19:44:41 -0400 Subject: [PATCH 08/12] Disable flag again --- packages/react/src/ActionList/Item.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index fc1be1a7caa..94b3cf8880f 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -61,7 +61,7 @@ export const Item = React.forwardRef( const {container, afterSelect, selectionAttribute, defaultTrailingVisual} = React.useContext(ActionListContainerContext) - const buttonSemanticsFeatureFlag = useFeatureFlag('primer_react_action_list_item_as_button') + const buttonSemanticsFeatureFlag = true // useFeatureFlag('primer_react_action_list_item_as_button') // Be sure to avoid rendering the container unless there is a default const wrappedDefaultTrailingVisual = defaultTrailingVisual ? ( From 0750e4afdaf8bfcb601d115d22632d023c3eb15f Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Mon, 30 Sep 2024 08:58:50 -0400 Subject: [PATCH 09/12] Remove `itemEventProps` --- packages/react/src/ActionList/Item.tsx | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index 94b3cf8880f..ce2a3636456 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -24,7 +24,7 @@ import VisuallyHidden from '../_VisuallyHidden' const LiBox = styled.li(sx) -const ButtonItemWrapper = React.forwardRef(({as: Component = 'button', children, styles, ...props}, forwardedRef) => { +const ButtonItemContainer = React.forwardRef(({as: Component = 'button', children, styles, ...props}, forwardedRef) => { return ( {children} @@ -283,7 +283,7 @@ export const Item = React.forwardRef( let DefaultItemWrapper = React.Fragment if (buttonSemanticsFeatureFlag) { - DefaultItemWrapper = listSemantics ? React.Fragment : ButtonItemWrapper + DefaultItemWrapper = listSemantics ? React.Fragment : ButtonItemContainer } const ItemWrapper = _PrivateItemWrapper || DefaultItemWrapper @@ -292,12 +292,9 @@ export const Item = React.forwardRef( const selectableRoles = ['menuitemradio', 'menuitemcheckbox', 'option'] const includeSelectionAttribute = itemSelectionAttribute && itemRole && selectableRoles.includes(itemRole) - const itemEventProps = { + const menuItemProps = { onClick: clickHandler, onKeyPress: !buttonSemantics ? keyPressHandler : undefined, - } - - const menuItemProps = { 'aria-disabled': disabled ? true : undefined, 'data-inactive': inactive ? true : undefined, 'data-loading': loading && !inactive ? true : undefined, @@ -311,7 +308,6 @@ export const Item = React.forwardRef( .join(' ') .trim() || undefined, ...(includeSelectionAttribute && {[itemSelectionAttribute]: selected}), - ...(buttonSemanticsFeatureFlag ? {} : itemEventProps), role: itemRole, id: itemId, } @@ -323,10 +319,10 @@ export const Item = React.forwardRef( containerProps = _PrivateItemWrapper ? {role: itemRole ? 'none' : undefined, ...props} : // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - (listSemantics && {...menuItemProps, ...itemEventProps, ...props, ref: forwardedRef}) || {...itemEventProps} + (listSemantics && {...menuItemProps, ...props, ref: forwardedRef}) || {} wrapperProps = _PrivateItemWrapper - ? {...menuItemProps, ...itemEventProps} + ? {...menuItemProps} : !listSemantics && { ...menuItemProps, ...props, From 9660adcfdda1af67495f2e19b8d384f6c30fe62a Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Mon, 30 Sep 2024 10:33:28 -0400 Subject: [PATCH 10/12] Enable back FF --- packages/react/src/ActionList/Item.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index ce2a3636456..c34cd4c1532 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -61,7 +61,7 @@ export const Item = React.forwardRef( const {container, afterSelect, selectionAttribute, defaultTrailingVisual} = React.useContext(ActionListContainerContext) - const buttonSemanticsFeatureFlag = true // useFeatureFlag('primer_react_action_list_item_as_button') + const buttonSemanticsFeatureFlag = useFeatureFlag('primer_react_action_list_item_as_button') // Be sure to avoid rendering the container unless there is a default const wrappedDefaultTrailingVisual = defaultTrailingVisual ? ( @@ -322,7 +322,7 @@ export const Item = React.forwardRef( (listSemantics && {...menuItemProps, ...props, ref: forwardedRef}) || {} wrapperProps = _PrivateItemWrapper - ? {...menuItemProps} + ? menuItemProps : !listSemantics && { ...menuItemProps, ...props, From 2ead1e4874bbd2ccdd16fe0d0821f4d123d7207c Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Mon, 30 Sep 2024 10:36:04 -0400 Subject: [PATCH 11/12] Add changeset --- .changeset/dull-moons-repair.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/dull-moons-repair.md diff --git a/.changeset/dull-moons-repair.md b/.changeset/dull-moons-repair.md new file mode 100644 index 00000000000..22d97582884 --- /dev/null +++ b/.changeset/dull-moons-repair.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +(Behind feature flag) ActionList: Add more guards for `ActionList.Item` before utilizing button semantics From 63816188a218645de10fe9fcb7773356fb9724c5 Mon Sep 17 00:00:00 2001 From: Tyler Jones Date: Tue, 1 Oct 2024 08:34:24 -0400 Subject: [PATCH 12/12] Update .changeset/dull-moons-repair.md Co-authored-by: Siddharth Kshetrapal --- .changeset/dull-moons-repair.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/dull-moons-repair.md b/.changeset/dull-moons-repair.md index 22d97582884..25257d13a72 100644 --- a/.changeset/dull-moons-repair.md +++ b/.changeset/dull-moons-repair.md @@ -2,4 +2,4 @@ '@primer/react': patch --- -(Behind feature flag) ActionList: Add more guards for `ActionList.Item` before utilizing button semantics +ActionList: Add more guards for `ActionList.Item` before utilizing button semantics (behind feature flag `primer_react_action_list_item_as_button`)