Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Menu: Arrow key navigation updates #1761

Merged
merged 1 commit into from
Jan 11, 2021
Merged

Menu: Arrow key navigation updates #1761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Binary file added packages/components/snapshots/MenuGroup/No Label-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
33 changes: 32 additions & 1 deletion packages/components/src/Menu/Menu.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -563,3 +564,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 },
}
6 changes: 6 additions & 0 deletions packages/components/src/Menu/MenuGroup.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ NoIcons.args = {
icons: false,
}

export const NoLabel = Template.bind({})
NoLabel.args = {
...Basic.args,
label: undefined,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this image snapshot to make up for a jest snapshot I removed.

export default {
component: MenuGroup,
title: 'MenuGroup',
Expand Down
95 changes: 34 additions & 61 deletions packages/components/src/Menu/MenuGroup.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing all the snapshots since they're covered by image snapshots and otherwise this PR would need to update them. Also added the describe – just cuz.

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()
})
})
32 changes: 12 additions & 20 deletions packages/components/src/Menu/MenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>) => {
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabIndex={-1} supports the "use arrows to move focus between items, tab to move focus in & out of the list" behavior.

>
{renderedIcon}
<span>
{children}
Expand All @@ -197,7 +190,6 @@ const MenuItemInternal: FC<MenuItemProps> = (props) => {
onBlur={handleOnBlur}
onClick={disabled ? undefined : handleOnClick}
onKeyUp={handleOnKeyUp}
onKeyDown={handleOnKeyDown}
className={className}
>
{tooltip ? (
Expand Down
4 changes: 1 addition & 3 deletions packages/components/src/Menu/MenuItemContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {}
Expand Down
33 changes: 14 additions & 19 deletions packages/components/src/Menu/MenuList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import React, {
Children,
forwardRef,
isValidElement,
KeyboardEvent,
ReactChild,
Ref,
useEffect,
Expand All @@ -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'

Expand Down Expand Up @@ -137,6 +136,10 @@ export const MenuListInternal = forwardRef(
pin,
placement,
windowing,

onBlur,
onFocus,
onKeyDown,
...props
}: MenuListProps,
forwardedRef: Ref<HTMLUListElement>
Expand Down Expand Up @@ -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>
Expand Down
Loading