Pre-ActionList CSS Modules bug fixes

.changeset/healthy-foxes-hunt.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch
+---
+
+ActionList UI bug fixes

e2e/components/ActionList.test.ts

@@ -657,34 +657,6 @@ test.describe('ActionList', () => {
     }
   })
 
-  test.describe('With Trailing Action', () => {
-    for (const theme of themes) {
-      test.describe(theme, () => {
-        test('default @vrt', async ({page}) => {
-          await visit(page, {
-            id: 'components-actionlist-features--with-trailing-action',
-            globals: {
-              colorScheme: theme,
-            },
-          })
-
-          // Default state
-          expect(await page.screenshot()).toMatchSnapshot(`ActionList.With Trailing Action.${theme}.png`)
-        })
-
-        test('axe @aat', async ({page}) => {
-          await visit(page, {
-            id: 'components-actionlist-features--with-trailing-action',
-            globals: {
-              colorScheme: theme,
-            },
-          })
-          await expect(page).toHaveNoViolations()
-        })
-      })
-    }
-  })
-
   test.describe('Full Variant', () => {
     for (const theme of themes) {
       test.describe(theme, () => {

e2e/components/NavList.test.ts

@@ -30,34 +30,4 @@ test.describe('NavList', () => {
       })
     }
   })
-
-  test.describe('With Bad Example of SubNav and TrailingAction', () => {
-    for (const theme of themes) {
-      test.describe(theme, () => {
-        test('default @vrt', async ({page}) => {
-          await visit(page, {
-            id: 'components-navlist-devonly--with-bad-example-of-sub-nav-and-trailing-action',
-            globals: {
-              colorScheme: theme,
-            },
-          })
-
-          // Default state
-          expect(await page.screenshot()).toMatchSnapshot(
-            `NavList.With Bad Example of SubNav and TrailingAction.${theme}.png`,
-          )
-        })
-
-        test('axe @aat', async ({page}) => {
-          await visit(page, {
-            id: 'components-navlist-devonly--with-bad-example-of-sub-nav-and-trailing-action',
-            globals: {
-              colorScheme: theme,
-            },
-          })
-          await expect(page).toHaveNoViolations()
-        })
-      })
-    }
-  })
 })

packages/react/src/ActionList/ActionList.features.stories.tsx

@@ -29,7 +29,6 @@ import {
   GitPullRequestIcon,
   IssueOpenedIcon,
   ProjectIcon,
-  LinkExternalIcon,
 } from '@primer/octicons-react'
 import {FeatureFlags} from '../FeatureFlags'
 
@@ -281,6 +280,7 @@ export const InactiveSingleSelect = () => {
   const [selectedIndex, setSelectedIndex] = React.useState(1)
   return (
     <ActionList selectionVariant="single" showDividers role="menu" aria-label="Project">
+      {/* menuitem because state is inactive */}
       <ActionList.Item role="menuitem" selected={false} inactiveText="Unavailable due to an outage">
         Inactive item
       </ActionList.Item>
@@ -409,6 +409,7 @@ export const InactiveMultiselect = () => {
   }
   return (
     <ActionList selectionVariant="multiple" role="menu" aria-label="Project">
+      {/* menuitem because state is inactive */}
       <ActionList.Item role="menuitem" selected={false} inactiveText="Unavailable due to an outage">
         Inactive item
       </ActionList.Item>
@@ -478,43 +479,41 @@ export const LoadingItem = () => {
 }
 
 export const Links = () => (
-  <>
+  <ActionList>
     <ActionList.Heading as="h1" sx={{fontSize: 1}}>
       Details
     </ActionList.Heading>
-    <ActionList>
-      <ActionList.LinkItem href="https://github.com/primer/react#readme">
-        <ActionList.LeadingVisual>
-          <BookIcon />
-        </ActionList.LeadingVisual>
-        Readme
-      </ActionList.LinkItem>
-      <ActionList.LinkItem href="https://github.com/primer/react/blob/main/LICENSE">
-        <ActionList.LeadingVisual>
-          <LawIcon />
-        </ActionList.LeadingVisual>
-        MIT License
-      </ActionList.LinkItem>
-      <ActionList.LinkItem href="https://github.com/primer/react/stargazers">
-        <ActionList.LeadingVisual>
-          <StarIcon />
-        </ActionList.LeadingVisual>
-        <strong>1.5k</strong> stars
-      </ActionList.LinkItem>
-      <ActionList.LinkItem href="https://github.com/primer/react/watchers">
-        <ActionList.LeadingVisual>
-          <EyeIcon />
-        </ActionList.LeadingVisual>
-        <strong>21</strong> watching
-      </ActionList.LinkItem>
-      <ActionList.LinkItem href="https://github.com/primer/react/network/members">
-        <ActionList.LeadingVisual>
-          <RepoForkedIcon />
-        </ActionList.LeadingVisual>
-        <strong>225</strong> forks
-      </ActionList.LinkItem>
-    </ActionList>
-  </>
+    <ActionList.LinkItem href="https://github.com/primer/react#readme">
+      <ActionList.LeadingVisual>
+        <BookIcon />
+      </ActionList.LeadingVisual>
+      Readme
+    </ActionList.LinkItem>
+    <ActionList.LinkItem href="https://github.com/primer/react/blob/main/LICENSE">
+      <ActionList.LeadingVisual>
+        <LawIcon />
+      </ActionList.LeadingVisual>
+      MIT License
+    </ActionList.LinkItem>
+    <ActionList.LinkItem href="https://github.com/primer/react/stargazers">
+      <ActionList.LeadingVisual>
+        <StarIcon />
+      </ActionList.LeadingVisual>
+      <strong>1.5k</strong> stars
+    </ActionList.LinkItem>
+    <ActionList.LinkItem href="https://github.com/primer/react/watchers">
+      <ActionList.LeadingVisual>
+        <EyeIcon />
+      </ActionList.LeadingVisual>
+      <strong>21</strong> watching
+    </ActionList.LinkItem>
+    <ActionList.LinkItem href="https://github.com/primer/react/network/members">
+      <ActionList.LeadingVisual>
+        <RepoForkedIcon />
+      </ActionList.LeadingVisual>
+      <strong>225</strong> forks
+    </ActionList.LinkItem>
+  </ActionList>
 )
 
 export const CustomItemChildren = () => (
@@ -796,85 +795,64 @@ export const WithCustomTrailingVisuals = () => (
   </ActionList>
 )
 
-export const ActionListWithButtonSemantics = () => {
-  return (
-    <FeatureFlags flags={{primer_react_action_list_item_as_button: true}}>
-      <ActionList>
-        <ActionList.Item>Copy link</ActionList.Item>
-        <ActionList.Item inactiveText="Nothing to quote">Quote reply</ActionList.Item>
-        <ActionList.Item disabled>Edit comment</ActionList.Item>
-        <ActionList.Divider />
-        <ActionList.Item variant="danger">Delete file</ActionList.Item>
-        <ActionList.LinkItem href="https://github.com/primer/react#readme">
-          Support
-          <ActionList.TrailingVisual>
-            <LinkExternalIcon />
-          </ActionList.TrailingVisual>
-        </ActionList.LinkItem>
-      </ActionList>
-    </FeatureFlags>
-  )
-}
-
-ActionListWithButtonSemantics.storyName = 'With Button Semantics (Behind feature flag)'
-
-export const WithTrailingAction = () => {
-  return (
-    <FeatureFlags flags={{primer_react_action_list_item_as_button: true}}>
-      <ActionList>
-        <ActionList.Item>
-          <ActionList.LeadingVisual>
-            <FileDirectoryIcon />
-          </ActionList.LeadingVisual>
-          Item 1 (with default TrailingAction)
-          <ActionList.TrailingAction label="Expand sidebar" icon={ArrowLeftIcon} />
-        </ActionList.Item>
-        <ActionList.Item>
-          Item 2 (with link TrailingAction)
-          <ActionList.TrailingAction as="a" href="#" label="Some action 1" icon={ArrowRightIcon} />
-        </ActionList.Item>
-        <ActionList.Item>
-          Item 3<ActionList.Description>This is an inline description.</ActionList.Description>
-          <ActionList.TrailingAction label="Some action 2" icon={BookIcon} />
-        </ActionList.Item>
-        <ActionList.Item>
-          Item 4<ActionList.Description variant="block">This is a block description.</ActionList.Description>
-          <ActionList.TrailingAction label="Some action 3" icon={BookIcon} />
-        </ActionList.Item>
-        <ActionList.Item>
-          Item 5<ActionList.Description variant="block">This is a block description.</ActionList.Description>
-          <ActionList.TrailingAction label="Some action 4" />
-        </ActionList.Item>
-        <ActionList.Item>
-          Item 6
-          <ActionList.TrailingAction href="#" as="a" label="Some action 5" />
-        </ActionList.Item>
-        <ActionList.LinkItem href="#">
-          LinkItem 1
-          <ActionList.Description>
-            with TrailingAction this is a long description and should not cause horizontal scroll on smaller screen
-            sizes
-          </ActionList.Description>
-          <ActionList.TrailingAction label="Another action" />
-        </ActionList.LinkItem>
-        <ActionList.LinkItem href="#">
-          LinkItem 2
-          <ActionList.Description>
-            with TrailingVisual this is a long description and should not cause horizontal scroll on smaller screen
-            sizes
-          </ActionList.Description>
-          <ActionList.TrailingVisual>
-            <TableIcon />
-          </ActionList.TrailingVisual>
-        </ActionList.LinkItem>
-        <ActionList.Item inactiveText="Unavailable due to an outage">
-          Inactive Item<ActionList.Description>With TrailingAction</ActionList.Description>
-          <ActionList.TrailingAction as="a" href="#" label="Some action 8" icon={ArrowRightIcon} />
-        </ActionList.Item>
-      </ActionList>
-    </FeatureFlags>
-  )
-}
+// removing this until CSS Modules FF ships, currently broken in production if button semantic FF is false
+// export const WithTrailingAction = () => {
+//   return (
+//     <FeatureFlags flags={{primer_react_action_list_item_as_button: true}}>
+//       <ActionList>
+//         <ActionList.Item>
+//           <ActionList.LeadingVisual>
+//             <FileDirectoryIcon />
+//           </ActionList.LeadingVisual>
+//           Item 1 (with default TrailingAction)
+//           <ActionList.TrailingAction label="Expand sidebar" icon={ArrowLeftIcon} />
+//         </ActionList.Item>
+//         <ActionList.Item>
+//           Item 2 (with link TrailingAction)
+//           <ActionList.TrailingAction as="a" href="#" label="Some action 1" icon={ArrowRightIcon} />
+//         </ActionList.Item>
+//         <ActionList.Item>
+//           Item 3<ActionList.Description>This is an inline description.</ActionList.Description>
+//           <ActionList.TrailingAction label="Some action 2" icon={BookIcon} />
+//         </ActionList.Item>
+//         <ActionList.Item>
+//           Item 4<ActionList.Description variant="block">This is a block description.</ActionList.Description>
+//           <ActionList.TrailingAction label="Some action 3" icon={BookIcon} />
+//         </ActionList.Item>
+//         <ActionList.Item>
+//           Item 5<ActionList.Description variant="block">This is a block description.</ActionList.Description>
+//           <ActionList.TrailingAction label="Some action 4" />
+//         </ActionList.Item>
+//         <ActionList.Item>
+//           Item 6
+//           <ActionList.TrailingAction href="#" as="a" label="Some action 5" />
+//         </ActionList.Item>
+//         <ActionList.LinkItem href="#">
+//           LinkItem 1
+//           <ActionList.Description>
+//             with TrailingAction this is a long description and should not cause horizontal scroll on smaller screen
+//             sizes
+//           </ActionList.Description>
+//           <ActionList.TrailingAction label="Another action" />
+//         </ActionList.LinkItem>
+//         <ActionList.LinkItem href="#">
+//           LinkItem 2
+//           <ActionList.Description>
+//             with TrailingVisual this is a long description and should not cause horizontal scroll on smaller screen
+//             sizes
+//           </ActionList.Description>
+//           <ActionList.TrailingVisual>
+//             <TableIcon />
+//           </ActionList.TrailingVisual>
+//         </ActionList.LinkItem>
+//         <ActionList.Item inactiveText="Unavailable due to an outage">
+//           Inactive Item<ActionList.Description>With TrailingAction</ActionList.Description>
+//           <ActionList.TrailingAction as="a" href="#" label="Some action 8" icon={ArrowRightIcon} />
+//         </ActionList.Item>
+//       </ActionList>
+//     </FeatureFlags>
+//   )
+// }
 
 export const FullVariant = () => (
   <ActionList variant="full">

packages/react/src/ActionList/ActionList.stories.tsx

@@ -220,6 +220,7 @@ LinkItemPlayground.args = {
   active: false,
   disabled: false,
   id: 'item-1',
+  variant: 'default',
   inactiveText: '',
   leadingVisual: null,
   loading: false,
@@ -231,6 +232,10 @@ LinkItemPlayground.argTypes = {
       type: 'boolean',
     },
   },
+  variant: {
+    control: 'radio',
+    options: ['default', 'danger'],
+  },
   role: {
     type: 'string',
   },

packages/react/src/ActionList/Divider.tsx

@@ -22,6 +22,8 @@ export const Divider: React.FC<React.PropsWithChildren<ActionListDividerProps>>
           marginTop: (theme: Theme) => `calc(${get('space.2')(theme)} - 1px)`,
           marginBottom: 2,
           listStyle: 'none', // hide the ::marker inserted by browser's stylesheet
+          marginRight: 'calc(-1 * var(--base-size-8, 8px))',
+          marginLeft: 'calc(-1 * var(--base-size-8, 8px))',
         },
         sx as SxProp,
       )}

packages/react/src/ActionList/Item.tsx

@@ -197,6 +197,9 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
           bg: selected ? 'fg.muted' : 'var(--control-bgColor-disabled, rgba(175, 184, 193, 0.2))',
           borderColor: selected ? 'fg.muted' : 'var(--color-input-disabled-bg, rgba(175, 184, 193, 0.2))',
         },
+        '[data-component="ActionList.Selection"]': {
+          color: 'primer.fg.disabled',
+        },
       },
 
       // Button reset styles (to support as="button")
@@ -393,6 +396,7 @@ export const Item = React.forwardRef<HTMLLIElement, ActionListItemProps>(
                       fontWeight: slots.inlineDescription || slots.blockDescription || active ? 'bold' : 'normal',
                       marginBlockEnd: slots.blockDescription ? '4px' : undefined,
                       wordBreak: slots.inlineDescription ? 'normal' : 'break-word',
+                      lineHeight: '20px',
                     }}
                   >
                     {childrenWithoutSlots}

packages/react/src/ActionList/Visuals.tsx

@@ -24,6 +24,7 @@ export const LeadingVisualContainer: React.FC<React.PropsWithChildren<VisualProp
           alignItems: 'center',
           flexShrink: 0,
           marginRight: 2,
+          color: 'fg.muted',
         },
         sx as SxProp,
       )}
@@ -69,6 +70,8 @@ export const TrailingVisual: React.FC<React.PropsWithChildren<VisualProps>> = ({
           color: getVariantStyles(variant, disabled, inactive).annotationColor,
           marginLeft: 2,
           fontWeight: 'initial',
+          display: 'grid',
+          alignContent: 'center',
           '[data-variant="danger"]:hover &, [data-variant="danger"]:active &': {
             color: getVariantStyles(variant, disabled, inactive).hoverColor,
           },

packages/react/src/ActionMenu/ActionMenu.features.stories.tsx

@@ -158,11 +158,7 @@ export const InactiveItems = () => (
             <GearIcon />
           </ActionList.LeadingVisual>
         </ActionList.LinkItem>
-        <ActionList.Item
-          variant="danger"
-          onSelect={() => alert('Make a copy clicked')}
-          inactiveText="Unavailable due to an outage"
-        >
+        <ActionList.Item onSelect={() => alert('Make a copy clicked')} inactiveText="Unavailable due to an outage">
           Make a copy
           <ActionList.LeadingVisual>
             <CopyIcon />