feat: Arrow key navigation persists selected item (Menu, Tabs) (#1761)

looker-open-source · Jan 11, 2021 · dc2400f · dc2400f
1 parent 300b873
commit dc2400f
Show file tree

Hide file tree

Showing 20 changed files with 448 additions and 744 deletions.
diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md
@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- `Tabs` and `Menu` arrow key navigation persists the last focused item
 - `Breakpoint` component
 - `Tree / TreeItem`
   - disabled and selected states

diff --git a/packages/components/snapshots/MenuGroup/No Label-snap.png b/packages/components/snapshots/MenuGroup/No Label-snap.png
diff --git a/packages/components/src/Menu/Menu.story.tsx b/packages/components/src/Menu/Menu.story.tsx
@@ -39,7 +39,8 @@ import { Divider } from '../Divider'
 import { FieldToggleSwitch } from '../Form'
 import { Icon } from '../Icon'
 import { Space, SpaceVertical } from '../Layout'
-import { Text, Paragraph } from '../Text'
+import { Tab, TabList, TabPanel, TabPanels, Tabs } from '../Tabs'
+import { Heading, Text, Paragraph } from '../Text'
 import { Tooltip } from '../Tooltip'
 import { useToggle } from '../utils'
 import { Menu } from './Menu'
@@ -564,3 +565,33 @@ export const WithTooltip = () => {
 WithTooltip.parameters = {
   storyshots: { disable: true },
 }
+
+export const ArrowKeyNavigation = () => (
+  <SpaceVertical align="start">
+    <Button>Above</Button>
+    <Heading>Menu</Heading>
+    <MenuList>
+      <MenuItem>1</MenuItem>
+      <MenuItem>2</MenuItem>
+      <MenuItem>3</MenuItem>
+    </MenuList>
+    <Heading>Tabs</Heading>
+    <Tabs>
+      <TabList>
+        <Tab>1</Tab>
+        <Tab>2</Tab>
+        <Tab>3</Tab>
+      </TabList>
+      <TabPanels>
+        <TabPanel>One</TabPanel>
+        <TabPanel>Two</TabPanel>
+        <TabPanel>Three</TabPanel>
+      </TabPanels>
+    </Tabs>
+    <Button>Below</Button>
+  </SpaceVertical>
+)
+
+ArrowKeyNavigation.parameters = {
+  storyshots: { disable: true },
+}
diff --git a/packages/components/src/Menu/MenuGroup.story.tsx b/packages/components/src/Menu/MenuGroup.story.tsx
@@ -96,6 +96,12 @@ NoIcons.args = {
   icons: false,
 }
 
+export const NoLabel = Template.bind({})
+NoLabel.args = {
+  ...Basic.args,
+  label: undefined,
+}
+
 export default {
   component: MenuGroup,
   title: 'MenuGroup',

diff --git a/packages/components/src/Menu/MenuGroup.test.tsx b/packages/components/src/Menu/MenuGroup.test.tsx
@@ -26,74 +26,47 @@
 
 import 'jest-styled-components'
 import React from 'react'
-import { assertSnapshot, renderWithTheme } from '@looker/components-test-utils'
+import { renderWithTheme } from '@looker/components-test-utils'
 
-import { Box } from '../Layout'
 import { MenuGroup } from './MenuGroup'
 import { MenuItem } from './MenuItem'
 import { MenuList } from './MenuList'
 
-test('MenuGroup', () => {
-  assertSnapshot(
-    <MenuGroup>
-      <MenuItem>who!</MenuItem>
-    </MenuGroup>
-  )
-})
-
-test('MenuGroup - label', () => {
-  assertSnapshot(
-    <MenuGroup label="Questions">
-      <MenuItem>what?</MenuItem>
-      <MenuItem>who?</MenuItem>
-      <MenuItem>where?</MenuItem>
-    </MenuGroup>
-  )
-})
-
-test('MenuGroup - JSX label', () => {
-  assertSnapshot(
-    <MenuGroup label={<Box>Questions</Box>}>
-      <MenuItem>what?</MenuItem>
-      <MenuItem>who?</MenuItem>
-      <MenuItem>where?</MenuItem>
-    </MenuGroup>
-  )
-})
-
-test('MenuGroup - indents MenuItem with no icon when a sibling within the same group has an icon', () => {
-  const { getByTestId } = renderWithTheme(
-    <MenuGroup>
-      <MenuItem icon="Calendar">Gouda</MenuItem>
-      <MenuItem id="cheddar">Cheddar</MenuItem>
-    </MenuGroup>
-  )
-
-  getByTestId('menu-item-cheddar-icon-placeholder')
-})
-
-test('MenuGroup - indents MenuItem with no icon when a sibling outside of the group has an icon', () => {
-  const { getByTestId } = renderWithTheme(
-    <MenuList>
-      <MenuItem icon="Calendar">Gouda</MenuItem>
+describe('MenuGroup', () => {
+  test('indents MenuItem with no icon when a sibling within the same group has an icon', () => {
+    const { getByTestId } = renderWithTheme(
       <MenuGroup>
+        <MenuItem icon="Calendar">Gouda</MenuItem>
         <MenuItem id="cheddar">Cheddar</MenuItem>
       </MenuGroup>
-    </MenuList>
-  )
-
-  getByTestId('menu-item-cheddar-icon-placeholder')
-})
-
-test('MenuGroup - does not indent MenuItems with no icon if all siblings do not have icons', () => {
-  const { queryByTestId } = renderWithTheme(
-    <MenuGroup>
-      <MenuItem>Gouda</MenuItem>
-      <MenuItem id="cheddar">Cheddar</MenuItem>
-    </MenuGroup>
-  )
+    )
+
+    getByTestId('menu-item-cheddar-icon-placeholder')
+  })
+
+  test('indents MenuItem with no icon when a sibling outside of the group has an icon', () => {
+    const { getByTestId } = renderWithTheme(
+      <MenuList>
+        <MenuItem icon="Calendar">Gouda</MenuItem>
+        <MenuGroup>
+          <MenuItem id="cheddar">Cheddar</MenuItem>
+        </MenuGroup>
+      </MenuList>
+    )
+
+    getByTestId('menu-item-cheddar-icon-placeholder')
+  })
+
+  test('does not indent MenuItems with no icon if all siblings do not have icons', () => {
+    const { queryByTestId } = renderWithTheme(
+      <MenuGroup>
+        <MenuItem>Gouda</MenuItem>
+        <MenuItem id="cheddar">Cheddar</MenuItem>
+      </MenuGroup>
+    )
 
-  expect(
-    queryByTestId('menu-item-cheddar-icon-placeholder')
-  ).not.toBeInTheDocument()
+    expect(
+      queryByTestId('menu-item-cheddar-icon-placeholder')
+    ).not.toBeInTheDocument()
+  })
 })
diff --git a/packages/components/src/Menu/MenuItem.tsx b/packages/components/src/Menu/MenuItem.tsx
@@ -86,7 +86,11 @@ const MenuItemInternal: FC<MenuItemProps> = (props) => {
   } = props
 
   const [isFocusVisible, setFocusVisible] = useState(false)
-  const { compact: contextCompact } = useContext(MenuItemContext)
+  const {
+    compact: contextCompact,
+    renderIconPlaceholder,
+    setRenderIconPlaceholder,
+  } = useContext(MenuItemContext)
   const compact = propCompact === undefined ? contextCompact : propCompact
 
   const handleOnBlur = (event: React.FocusEvent<HTMLLIElement>) => {
@@ -95,12 +99,6 @@ const MenuItemInternal: FC<MenuItemProps> = (props) => {
   }
 
   const { closeModal } = useContext(DialogContext)
-  const {
-    renderIconPlaceholder,
-    setRenderIconPlaceholder,
-    handleArrowDown,
-    handleArrowUp,
-  } = useContext(MenuItemContext)
 
   const handleOnClick = (event: React.MouseEvent<HTMLLIElement>) => {
     setFocusVisible(false)
@@ -111,17 +109,6 @@ const MenuItemInternal: FC<MenuItemProps> = (props) => {
     }
   }
 
-  const handleOnKeyDown = (event: React.KeyboardEvent<HTMLLIElement>) => {
-    switch (event.key) {
-      case 'ArrowUp':
-        handleArrowUp && handleArrowUp(event)
-        break
-      case 'ArrowDown':
-        handleArrowDown && handleArrowDown(event)
-        break
-    }
-  }
-
   const handleOnKeyUp = (event: React.KeyboardEvent<HTMLLIElement>) => {
     onKeyUp && onKeyUp(event)
     setFocusVisible(true)
@@ -174,7 +161,13 @@ const MenuItemInternal: FC<MenuItemProps> = (props) => {
       : props.rel
 
   const menuItemContent = (
-    <Component href={href} rel={rel} role="menuitem" target={target}>
+    <Component
+      href={href}
+      rel={rel}
+      role="menuitem"
+      target={target}
+      tabIndex={-1}
+    >
       {renderedIcon}
       <span>
         {children}
@@ -197,7 +190,6 @@ const MenuItemInternal: FC<MenuItemProps> = (props) => {
       onBlur={handleOnBlur}
       onClick={disabled ? undefined : handleOnClick}
       onKeyUp={handleOnKeyUp}
-      onKeyDown={handleOnKeyDown}
       className={className}
     >
       {tooltip ? (

diff --git a/packages/components/src/Menu/MenuItemContext.ts b/packages/components/src/Menu/MenuItemContext.ts
@@ -24,14 +24,12 @@
 
  */
 
-import { createContext, KeyboardEvent } from 'react'
+import { createContext } from 'react'
 
 export interface MenuItemContextProps {
   compact?: boolean
   renderIconPlaceholder?: boolean
   setRenderIconPlaceholder?: (state: boolean) => void
-  handleArrowUp?: (e: KeyboardEvent<HTMLLIElement>) => void
-  handleArrowDown?: (e: KeyboardEvent<HTMLLIElement>) => void
 }
 
 const menuItemContext: MenuItemContextProps = {}

diff --git a/packages/components/src/Menu/MenuList.tsx b/packages/components/src/Menu/MenuList.tsx
@@ -30,7 +30,6 @@ import React, {
   Children,
   forwardRef,
   isValidElement,
-  KeyboardEvent,
   ReactChild,
   Ref,
   useEffect,
@@ -57,7 +56,7 @@ import {
   reset,
   omitStyledProps,
 } from '@looker/design-tokens'
-import { moveFocus, useForkedRef, useWindow } from '../utils'
+import { useArrowKeyNav, useWindow } from '../utils'
 import { MenuItemContext } from './MenuItemContext'
 import { MenuGroup } from './MenuGroup'
 
@@ -137,6 +136,10 @@ export const MenuListInternal = forwardRef(
       pin,
       placement,
       windowing,
+
+      onBlur,
+      onFocus,
+      onKeyDown,
       ...props
     }: MenuListProps,
     forwardedRef: Ref<HTMLUListElement>
@@ -179,41 +182,33 @@ export const MenuListInternal = forwardRef(
       return (child: ReactChild) => getMenuGroupHeight(child, compact)
     }, [windowing, childArray, compact])
 
-    const { content, containerElement, ref } = useWindow({
+    const { content, ref } = useWindow({
       childHeight: childHeight,
       children: children as JSX.Element | JSX.Element[],
       enabled: windowing !== 'none',
+      ref: forwardedRef,
       spacerTag: 'li',
     })
-    const forkedRef = useForkedRef(forwardedRef, ref)
 
-    function handleArrowKey(direction: number, initial: number) {
-      moveFocus(direction, initial, containerElement)
-    }
+    const navProps = useArrowKeyNav({
+      onBlur,
+      onFocus,
+      onKeyDown,
+      ref,
+    })
 
     const context = {
       compact,
-      handleArrowDown: (e: KeyboardEvent<HTMLLIElement>) => {
-        e.preventDefault()
-        handleArrowKey(1, 0)
-        return false
-      },
-      handleArrowUp: (e: KeyboardEvent<HTMLLIElement>) => {
-        e.preventDefault()
-        handleArrowKey(-1, -1)
-        return false
-      },
       renderIconPlaceholder,
       setRenderIconPlaceholder,
     }
 
     return (
       <MenuItemContext.Provider value={context}>
         <ul
-          ref={forkedRef}
-          tabIndex={-1}
           role="menu"
           {...omitStyledProps(omit(props, 'groupDividers'))}
+          {...navProps}
         >
           {content}
         </ul>