Merge DropdownMenu2 into ActionMenu2 (#1848)

* merge docs * merge stories/examples * merge stories/fixtures * move itemRole to item and delete dropdonwmenu * add test for single selection * Add tests for additional use cases * delete outdated snapshot * add changeset * fix group roles based on latest discussion * Revert "fix group roles based on latest discussion" This reverts commit 1e90bda. * Update docs/content/drafts/ActionMenu2.mdx Co-authored-by: Cole Bemis <colebemis@github.com> * use merged ActionMenu instead of Dropdown in Overlay * bring back DropdownMenu2 part 1/2 * bring back DropdownMenu2 part 2/2 Co-authored-by: Cole Bemis <colebemis@github.com>
primer · Feb 21, 2022 · 96a151a · 96a151a
1 parent b22a795
commit 96a151a
Show file tree

Hide file tree

Showing 15 changed files with 547 additions and 174 deletions.
diff --git a/.changeset/drafts-dropdownmenu2-merged-into-actionmenu2.md b/.changeset/drafts-dropdownmenu2-merged-into-actionmenu2.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Merges drafts/DropdownMenu2 into drafts/ActionMenu2
diff --git a/docs/content/drafts/ActionMenu2.mdx b/docs/content/drafts/ActionMenu2.mdx
@@ -175,6 +175,44 @@ You can choose to have a different _anchor_ for the Menu dependending on the app
 </ActionMenu>
 ```
 
+### With selection
+
+Use `selectionVariant` on `ActionList` to create a menu with single or multiple selection.
+
+```javascript live noinline drafts
+const fieldTypes = [
+  {icon: TypographyIcon, name: 'Text'},
+  {icon: NumberIcon, name: 'Number'},
+  {icon: CalendarIcon, name: 'Date'},
+  {icon: SingleSelectIcon, name: 'Single select'},
+  {icon: IterationsIcon, name: 'Iteration'}
+]
+
+const Example = () => {
+  const [selectedIndex, setSelectedIndex] = React.useState(1)
+  const selectedType = fieldTypes[selectedIndex]
+
+  return (
+    <ActionMenu>
+      <ActionMenu.Button aria-label="Select field type" leadingIcon={selectedType.icon}>
+        {selectedType.name}
+      </ActionMenu.Button>
+      <ActionMenu.Overlay width="medium">
+        <ActionList selectionVariant="single">
+          {fieldTypes.map((type, index) => (
+            <ActionList.Item key={index} selected={index === selectedIndex} onSelect={() => setSelectedIndex(index)}>
+              <type.icon /> {type.name}
+            </ActionList.Item>
+          ))}
+        </ActionList>
+      </ActionMenu.Overlay>
+    </ActionMenu>
+  )
+}
+
+render(<Example />)
+```
+
 ### With External Anchor
 
 To create an anchor outside of the menu, you need to switch to controlled mode for the menu and pass it as `anchorRef` to `ActionMenu`. Make sure you add `aria-expanded` and `aria-haspopup` to the external anchor:
@@ -241,12 +279,6 @@ render(
 )
 ```
 
-<Note variant="warning">
-
-Use `ActionMenu` to choose an action from a list. If you’re looking for single or multiple selection, use [DropdownMenu](/DropdownMenu) or [SelectPanel](/SelectPanel) instead.
-
-</Note>
-
 ## Props / API reference
 
 ### ActionMenu
@@ -305,6 +337,5 @@ Use `ActionMenu` to choose an action from a list. If you’re looking for single
 ## Related components
 
 - [ActionList](/drafts/ActionList2)
-- [DropdownMenu](/DropdownMenu)
 - [SelectPanel](/SelectPanel)
 - [Button](/drafts/Button2)
diff --git a/src/ActionList2/ActionListContainerContext.tsx b/src/ActionList2/ActionListContainerContext.tsx
@@ -5,8 +5,7 @@ import React from 'react'
 type ContextProps = {
   container?: string
   listRole?: string
-  itemRole?: string
-  selectionVariant?: 'single' | 'multiple'
+  selectionVariant?: 'single' | 'multiple' // TODO: Remove after DropdownMenu2 deprecation
   selectionAttribute?: 'aria-selected' | 'aria-checked'
   listLabelledBy?: string
   // This can be any function, we don't know anything about the arguments

diff --git a/src/ActionList2/Item.tsx b/src/ActionList2/Item.tsx
@@ -7,7 +7,8 @@ import Box, {BoxProps} from '../Box'
 import sx, {SxProp, merge} from '../sx'
 import createSlots from '../utils/create-slots'
 import {AriaRole} from '../utils/types'
-import {ListContext} from './List'
+import {ListContext, ListProps} from './List'
+import {GroupContext, GroupProps} from './Group'
 import {ActionListContainerContext} from './ActionListContainerContext'
 import {Selection} from './Selection'
 
@@ -101,8 +102,21 @@ export const Item = React.forwardRef<HTMLLIElement, ItemProps>(
     },
     forwardedRef
   ): JSX.Element => {
-    const {variant: listVariant, showDividers} = React.useContext(ListContext)
-    const {itemRole, afterSelect, selectionAttribute = 'aria-selected'} = React.useContext(ActionListContainerContext)
+    const {variant: listVariant, showDividers, selectionVariant: listSelectionVariant} = React.useContext(ListContext)
+    const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext)
+    const {container, afterSelect, selectionAttribute = 'aria-selected'} = React.useContext(ActionListContainerContext)
+
+    let selectionVariant: ListProps['selectionVariant'] | GroupProps['selectionVariant']
+    if (typeof groupSelectionVariant !== 'undefined') selectionVariant = groupSelectionVariant
+    else selectionVariant = listSelectionVariant
+
+    /** Infer item role based on the container */
+    let itemRole: ItemProps['role']
+    if (container === 'ActionMenu' || container === 'DropdownMenu') {
+      if (selectionVariant === 'single') itemRole = 'menuitemradio'
+      else if (selectionVariant === 'multiple') itemRole = 'menuitemcheckbox'
+      else itemRole = 'menuitem'
+    }
 
     const {theme} = useTheme()
 

diff --git a/src/ActionList2/List.tsx b/src/ActionList2/List.tsx
@@ -41,7 +41,11 @@ export const List = React.forwardRef<HTMLUListElement, ListProps>(
     }
 
     /** if list is inside a Menu, it will get a role from the Menu */
-    const {listRole, listLabelledBy} = React.useContext(ActionListContainerContext)
+    const {
+      listRole,
+      listLabelledBy,
+      selectionVariant: containerSelectionVariant // TODO: Remove after DropdownMenu2 deprecation
+    } = React.useContext(ActionListContainerContext)
 
     return (
       <ListBox
@@ -51,7 +55,11 @@ export const List = React.forwardRef<HTMLUListElement, ListProps>(
         {...props}
         ref={forwardedRef}
       >
-        <ListContext.Provider value={{variant, selectionVariant, showDividers}}>{props.children}</ListContext.Provider>
+        <ListContext.Provider
+          value={{variant, selectionVariant: selectionVariant || containerSelectionVariant, showDividers}}
+        >
+          {props.children}
+        </ListContext.Provider>
       </ListBox>
     )
   }

diff --git a/src/ActionList2/Selection.tsx b/src/ActionList2/Selection.tsx
@@ -1,43 +1,31 @@
 import React from 'react'
 import {CheckIcon} from '@primer/octicons-react'
-import {ListContext} from './List'
-import {GroupContext} from './Group'
-import {ActionListContainerContext} from './ActionListContainerContext'
+import {ListContext, ListProps} from './List'
+import {GroupContext, GroupProps} from './Group'
 import {ItemProps} from './Item'
 import {LeadingVisualContainer} from './Visuals'
 
 type SelectionProps = Pick<ItemProps, 'selected'>
 export const Selection: React.FC<SelectionProps> = ({selected}) => {
   const {selectionVariant: listSelectionVariant} = React.useContext(ListContext)
   const {selectionVariant: groupSelectionVariant} = React.useContext(GroupContext)
-  const {container, selectionVariant: menuSelectionVariant} = React.useContext(ActionListContainerContext)
 
   /** selectionVariant in Group can override the selectionVariant in List root */
-  /** fallback to selectionVariant from container menu if any (ActionMenu, DropdownMenu, SelectPanel ) */
-  let selectionVariant
+  /** fallback to selectionVariant from container menu if any (ActionMenu, SelectPanel ) */
+  let selectionVariant: ListProps['selectionVariant'] | GroupProps['selectionVariant']
   if (typeof groupSelectionVariant !== 'undefined') selectionVariant = groupSelectionVariant
-  else selectionVariant = listSelectionVariant || menuSelectionVariant
+  else selectionVariant = listSelectionVariant
 
-  // if selectionVariant is not set on List, don't show selection
   if (!selectionVariant) {
-    // to avoid confusion, fail loudly instead of silently ignoring
-    if (selected)
+    // if selectionVariant is not set on List, but Item is selected
+    // fail loudly instead of silently ignoring
+    if (selected) {
       throw new Error(
         'For Item to be selected, ActionList or ActionList.Group needs to have a selectionVariant defined'
       )
-    return null
-  }
-
-  if (container === 'ActionMenu') {
-    throw new Error(
-      'ActionList cannot have a selectionVariant inside ActionMenu, please use DropdownMenu or SelectPanel instead. More information: https://primer.style/design/components/action-list#application'
-    )
-  }
-
-  if (container === 'DropdownMenu' && selectionVariant === 'multiple') {
-    throw new Error(
-      'selectionVariant multiple cannot be used in DropdownMenu, please use SelectPanel instead. More information: https://primer.style/design/components/action-list#application'
-    )
+    } else {
+      return null
+    }
   }
 
   if (selectionVariant === 'single') {

diff --git a/src/ActionMenu2.tsx b/src/ActionMenu2.tsx
@@ -113,8 +113,8 @@ const Overlay: React.FC<MenuOverlayProps> = ({children, ...overlayProps}) => {
           value={{
             container: 'ActionMenu',
             listRole: 'menu',
-            itemRole: 'menuitem',
             listLabelledBy: anchorId,
+            selectionAttribute: 'aria-checked', // Should this be here?
             afterSelect: onClose
           }}
         >

diff --git a/src/DropdownMenu2.tsx b/src/DropdownMenu2.tsx
@@ -117,7 +117,6 @@ const Overlay: React.FC<MenuOverlayProps> = ({children, ...overlayProps}) => {
           value={{
             container: 'DropdownMenu',
             listRole: 'menu',
-            itemRole: 'menuitemradio',
             listLabelledBy: anchorId,
             selectionVariant: 'single',
             selectionAttribute: 'aria-checked',

diff --git a/src/__tests__/ActionMenu2.test.tsx b/src/__tests__/ActionMenu2.test.tsx
@@ -7,10 +7,11 @@ import {ActionMenu} from '../ActionMenu2'
 import {ActionList} from '../ActionList2'
 import {behavesAsComponent, checkExports, checkStoriesForAxeViolations} from '../utils/testing'
 import {BaseStyles, ThemeProvider, SSRProvider} from '..'
+import {SingleSelection, MixedSelection} from '../../src/stories/ActionMenu2/examples.stories'
 import '@testing-library/jest-dom'
 expect.extend(toHaveNoViolations)
 
-function SimpleActionMenu(): JSX.Element {
+function Example(): JSX.Element {
   return (
     <ThemeProvider theme={theme}>
       <SSRProvider>
@@ -39,7 +40,7 @@ describe('ActionMenu', () => {
   behavesAsComponent({
     Component: ActionList,
     options: {skipAs: true, skipSx: true},
-    toRender: () => <SimpleActionMenu />
+    toRender: () => <Example />
   })
 
   checkExports('ActionMenu2', {
@@ -48,15 +49,15 @@ describe('ActionMenu', () => {
   })
 
   it('should open Menu on MenuButton click', async () => {
-    const component = HTMLRender(<SimpleActionMenu />)
+    const component = HTMLRender(<Example />)
     const button = component.getByText('Toggle Menu')
     fireEvent.click(button)
     expect(component.getByRole('menu')).toBeInTheDocument()
     cleanup()
   })
 
   it('should open Menu on MenuButton keypress', async () => {
-    const component = HTMLRender(<SimpleActionMenu />)
+    const component = HTMLRender(<Example />)
     const button = component.getByText('Toggle Menu')
 
     // We pass keycode here to navigate a implementation detail in react-testing-library
@@ -67,7 +68,7 @@ describe('ActionMenu', () => {
   })
 
   it('should close Menu on selecting an action with click', async () => {
-    const component = HTMLRender(<SimpleActionMenu />)
+    const component = HTMLRender(<Example />)
     const button = component.getByText('Toggle Menu')
 
     fireEvent.click(button)
@@ -79,7 +80,7 @@ describe('ActionMenu', () => {
   })
 
   it('should close Menu on selecting an action with Enter', async () => {
-    const component = HTMLRender(<SimpleActionMenu />)
+    const component = HTMLRender(<Example />)
     const button = component.getByText('Toggle Menu')
 
     fireEvent.click(button)
@@ -91,7 +92,7 @@ describe('ActionMenu', () => {
   })
 
   it('should not close Menu if event is prevented', async () => {
-    const component = HTMLRender(<SimpleActionMenu />)
+    const component = HTMLRender(<Example />)
     const button = component.getByText('Toggle Menu')
 
     fireEvent.click(button)
@@ -103,38 +104,42 @@ describe('ActionMenu', () => {
     cleanup()
   })
 
-  it('should throw when selectionVariant is provided to ActionList within ActionMenu', 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(() => {
-      const component = HTMLRender(
-        <ThemeProvider theme={theme}>
-          <SSRProvider>
-            <BaseStyles>
-              <ActionMenu>
-                <ActionMenu.Button>Toggle Menu</ActionMenu.Button>
-                <ActionMenu.Overlay>
-                  <ActionList selectionVariant="single">
-                    <ActionList.Item selected={true}>Primer React</ActionList.Item>
-                  </ActionList>
-                </ActionMenu.Overlay>
-              </ActionMenu>
-            </BaseStyles>
-          </SSRProvider>
-        </ThemeProvider>
-      )
-
-      const button = component.getByText('Toggle Menu')
-      fireEvent.click(button)
-    }).toThrow('ActionList cannot have a selectionVariant inside ActionMenu')
+  it('should be able to select an Item with selectionVariant', async () => {
+    const component = HTMLRender(
+      <ThemeProvider theme={theme}>
+        <SingleSelection />
+      </ThemeProvider>
+    )
+    const button = component.getByLabelText('Select field type')
+    fireEvent.click(button)
+
+    // select first item by role, that would close the menu
+    fireEvent.click(component.getAllByRole('menuitemradio')[0])
+    expect(component.queryByRole('menu')).not.toBeInTheDocument()
+
+    // open menu again and check if the first option is checked
+    fireEvent.click(button)
+    expect(component.getAllByRole('menuitemradio')[0]).toHaveAttribute('aria-checked', 'true')
+    cleanup()
+  })
+
+  it('should assign the right roles with groups & mixed selectionVariant', async () => {
+    const component = HTMLRender(
+      <ThemeProvider theme={theme}>
+        <MixedSelection />
+      </ThemeProvider>
+    )
+    const button = component.getByLabelText('Select field type to group by')
+    fireEvent.click(button)
+
+    expect(component.getByLabelText('Status')).toHaveAttribute('role', 'menuitemradio')
+    expect(component.getByLabelText('Clear Group by')).toHaveAttribute('role', 'menuitem')
 
     cleanup()
-    mockError.mockRestore()
   })
 
   it('should have no axe violations', async () => {
-    const {container} = HTMLRender(<SimpleActionMenu />)
+    const {container} = HTMLRender(<Example />)
     const results = await axe(container)
     expect(results).toHaveNoViolations()
     cleanup()

diff --git a/src/__tests__/DropdownMenu2.test.tsx b/src/__tests__/DropdownMenu2.test.tsx
@@ -96,36 +96,6 @@ describe('DropdownMenu', () => {
     cleanup()
   })
 
-  it('should throw when selectionVariant=multiple is provided to ActionList within DropdownMenu', 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(() => {
-      const component = HTMLRender(
-        <ThemeProvider theme={theme}>
-          <SSRProvider>
-            <BaseStyles>
-              <DropdownMenu>
-                <DropdownMenu.Button>Select a field</DropdownMenu.Button>
-                <DropdownMenu.Overlay>
-                  <ActionList selectionVariant="multiple">
-                    <ActionList.Item selected={true}>Primer React</ActionList.Item>
-                  </ActionList>
-                </DropdownMenu.Overlay>
-              </DropdownMenu>
-            </BaseStyles>
-          </SSRProvider>
-        </ThemeProvider>
-      )
-
-      const button = component.getByText('Select a field')
-      fireEvent.click(button)
-    }).toThrow('selectionVariant multiple cannot be used in DropdownMenu')
-
-    cleanup()
-    mockError.mockRestore()
-  })
-
   checkStoriesForAxeViolations('DropdownMenu2/fixtures')
   checkStoriesForAxeViolations('DropdownMenu2/examples')
 })