ActionList v2: 100% test coverage (#1663)

* delete unused component

* add tests for onSelect

* Add tests for onSelect branches

* Fix story name in story utility

* Fix TS error in util

* make istanbul happy

* Add disabled inline items to story

* tiny refactor that's cleaner while passing tests

src/ActionList2/Item.tsx

@@ -91,7 +91,7 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
       variant = 'default',
       disabled = false,
       selected = undefined,
-      onSelect = () => null,
+      onSelect,
       sx: sxProp = {},
       id,
       _PrivateItemWrapper,
@@ -168,6 +168,7 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
 
     const clickHandler = React.useCallback(
       event => {
+        if (typeof onSelect !== 'function') return
         if (disabled) return
         if (!event.defaultPrevented) onSelect(event)
       },
@@ -176,8 +177,8 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
 
     const keyPressHandler = React.useCallback(
       event => {
+        if (typeof onSelect !== 'function') return
         if (disabled) return
-
         if (!event.defaultPrevented && [' ', 'Enter'].includes(event.key)) {
           onSelect(event)
         }

src/__tests__/ActionList2.test.tsx

@@ -1,11 +1,12 @@
-import {cleanup, render as HTMLRender} from '@testing-library/react'
+import {cleanup, render as HTMLRender, waitFor, fireEvent} from '@testing-library/react'
 import 'babel-polyfill'
 import {axe, toHaveNoViolations} from 'jest-axe'
 import React from 'react'
 import theme from '../theme'
 import {ActionList} from '../ActionList2'
 import {behavesAsComponent, checkExports, checkStoriesForAxeViolations} from '../utils/testing'
 import {BaseStyles, ThemeProvider, SSRProvider} from '..'
+import '@testing-library/jest-dom'
 expect.extend(toHaveNoViolations)
 
 function SimpleActionList(): JSX.Element {
@@ -26,6 +27,31 @@ function SimpleActionList(): JSX.Element {
   )
 }
 
+const projects = [
+  {name: 'Primer Backlog', scope: 'GitHub'},
+  {name: 'Primer React', scope: 'github/primer'},
+  {name: 'Disabled Project', scope: 'github/primer', disabled: true}
+]
+function SingleSelectListStory(): JSX.Element {
+  const [selectedIndex, setSelectedIndex] = React.useState(0)
+
+  return (
+    <ActionList selectionVariant="single" showDividers role="listbox" aria-label="Select a project">
+      {projects.map((project, index) => (
+        <ActionList.Item
+          key={index}
+          role="option"
+          selected={index === selectedIndex}
+          onSelect={() => setSelectedIndex(index)}
+          disabled={project.disabled}
+        >
+          {project.name}
+        </ActionList.Item>
+      ))}
+    </ActionList>
+  )
+}
+
 describe('ActionList', () => {
   behavesAsComponent({
     Component: ActionList,
@@ -45,5 +71,86 @@ describe('ActionList', () => {
     cleanup()
   })
 
+  it('should fire onSelect on click and keypress', async () => {
+    const component = HTMLRender(<SingleSelectListStory />)
+    const options = await waitFor(() => component.getAllByRole('option'))
+
+    expect(options[0]).toHaveAttribute('aria-selected', 'true')
+    expect(options[1]).toHaveAttribute('aria-selected', 'false')
+
+    fireEvent.click(options[1])
+
+    expect(options[0]).toHaveAttribute('aria-selected', 'false')
+    expect(options[1]).toHaveAttribute('aria-selected', 'true')
+
+    // We pass keycode here to navigate a implementation detail in react-testing-library
+    // https://github.com/testing-library/react-testing-library/issues/269#issuecomment-455854112
+    fireEvent.keyPress(options[0], {key: 'Enter', charCode: 13})
+
+    expect(options[0]).toHaveAttribute('aria-selected', 'true')
+    expect(options[1]).toHaveAttribute('aria-selected', 'false')
+
+    fireEvent.keyPress(options[1], {key: ' ', charCode: 32})
+
+    expect(options[0]).toHaveAttribute('aria-selected', 'false')
+    expect(options[1]).toHaveAttribute('aria-selected', 'true')
+
+    cleanup()
+  })
+
+  it('should skip onSelect on disabled items', async () => {
+    const component = HTMLRender(<SingleSelectListStory />)
+    const options = await waitFor(() => component.getAllByRole('option'))
+
+    expect(options[0]).toHaveAttribute('aria-selected', 'true')
+    expect(options[2]).toHaveAttribute('aria-selected', 'false')
+
+    fireEvent.click(options[2])
+
+    expect(options[0]).toHaveAttribute('aria-selected', 'true')
+    expect(options[2]).toHaveAttribute('aria-selected', 'false')
+
+    fireEvent.keyPress(options[2], {key: 'Enter', charCode: 13})
+
+    expect(options[0]).toHaveAttribute('aria-selected', 'true')
+    expect(options[2]).toHaveAttribute('aria-selected', 'false')
+
+    cleanup()
+  })
+
+  it('should throw when selected is provided without a selectionVariant on parent', async () => {
+    // we expect console.error to be called, so we suppress that in the test
+    const mockError = jest.spyOn(console, 'error').mockImplementation(() => jest.fn())
+
+    expect(() => {
+      HTMLRender(
+        <ActionList showDividers role="listbox" aria-label="Select a project">
+          <ActionList.Item role="option" selected={true}>
+            Primer React
+          </ActionList.Item>
+        </ActionList>
+      )
+    }).toThrow('For Item to be selected, ActionList or ActionList.Group needs to have a selectionVariant defined')
+
+    cleanup()
+    mockError.mockRestore()
+  })
+
+  it('should not crash when clicking an item without an onSelect', async () => {
+    const component = HTMLRender(
+      <ActionList role="listbox">
+        <ActionList.Item role="option">Primer React</ActionList.Item>
+      </ActionList>
+    )
+    const option = await waitFor(() => component.getByRole('option'))
+    expect(option).toBeInTheDocument()
+
+    fireEvent.click(option)
+    fireEvent.keyPress(option, {key: 'Enter', charCode: 13})
+    expect(option).toBeInTheDocument()
+
+    cleanup()
+  })
+
   checkStoriesForAxeViolations('ActionList2')
 })

src/stories/ActionList2.stories.tsx

@@ -801,7 +801,7 @@ export function AllCombinations(): JSX.Element {
             L + I + B<ActionList.Description variant="inline">inline description</ActionList.Description>
             <ActionList.Description variant="block">Block description</ActionList.Description>
           </ActionList.Item>
-          <ActionList.Item>
+          <ActionList.Item disabled>
             <ActionList.LeadingVisual>
               <StarIcon />
             </ActionList.LeadingVisual>
@@ -810,7 +810,7 @@ export function AllCombinations(): JSX.Element {
               <StarIcon />
             </ActionList.TrailingVisual>
           </ActionList.Item>
-          <ActionList.Item>
+          <ActionList.Item disabled>
             <ActionList.LeadingVisual>
               <StarIcon />
             </ActionList.LeadingVisual>
@@ -819,7 +819,7 @@ export function AllCombinations(): JSX.Element {
               <StarIcon />
             </ActionList.TrailingVisual>
           </ActionList.Item>
-          <ActionList.Item>
+          <ActionList.Item disabled>
             I + B + T<ActionList.Description variant="inline">inline description</ActionList.Description>
             <ActionList.Description variant="block">Block description</ActionList.Description>
             <ActionList.TrailingVisual>

src/utils/testing.tsx

@@ -5,6 +5,7 @@ import enzyme from 'enzyme'
 import Adapter from '@wojtekmaj/enzyme-adapter-react-17'
 import {cleanup, render as HTMLRender} from '@testing-library/react'
 import {axe, toHaveNoViolations} from 'jest-axe'
+import type {Story as StoryType} from '@storybook/react'
 import {ThemeProvider} from '..'
 import {default as defaultTheme} from '../theme'
 
@@ -253,7 +254,7 @@ export function checkStoriesForAxeViolations(name: string) {
   Object.values(Stories).map(Story => {
     if (typeof Story !== 'function') return
 
-    it(`story {Story.storyName} should have no axe violations`, async () => {
+    it(`story ${(Story as StoryType).storyName} should have no axe violations`, async () => {
       const {container} = HTMLRender(<Story />)
       const results = await axe(container)
       expect(results).toHaveNoViolations()