From 107fe1564f56baedeeb8cf4c4d04f033e8983571 Mon Sep 17 00:00:00 2001 From: Wes Souza Date: Mon, 8 Aug 2022 18:23:45 -0400 Subject: [PATCH] refactor(select): improve keyboard and events, split native to SelectNative BREAKING CHANGE: Select props changed significantly, native is now the SelectNative component --- package.json | 3 +- src/DatePicker/DatePicker.tsx | 10 +- src/Select/Select.spec.tsx | 821 ++++++++++++++++++++---------- src/Select/Select.stories.data.ts | 153 ++++++ src/Select/Select.stories.tsx | 62 +-- src/Select/Select.styles.tsx | 11 +- src/Select/Select.tsx | 650 ++++++----------------- src/Select/Select.types.ts | 40 +- src/Select/SelectNative.spec.tsx | 75 +++ src/Select/SelectNative.tsx | 85 ++++ src/Select/useSelectCommon.tsx | 82 +++ src/Select/useSelectState.ts | 591 +++++++++++++++++++++ src/common/constants.ts | 12 + src/common/hooks/useId.spec.ts | 14 + src/common/hooks/useId.ts | 15 + src/types.ts | 3 + yarn.lock | 15 + 17 files changed, 1845 insertions(+), 797 deletions(-) create mode 100644 src/Select/Select.stories.data.ts create mode 100644 src/Select/SelectNative.spec.tsx create mode 100644 src/Select/SelectNative.tsx create mode 100644 src/Select/useSelectCommon.tsx create mode 100644 src/Select/useSelectState.ts create mode 100644 src/common/constants.ts create mode 100644 src/common/hooks/useId.spec.ts create mode 100644 src/common/hooks/useId.ts diff --git a/package.json b/package.json index f530d16a..1ae71b9f 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "access": "public" }, "scripts": { - "start": "cross-env STORYBOOK_DISPLAY_WARNING=true DISPLAY_WARNING=true start-storybook -p 9009", + "start": "cross-env STORYBOOK_DISPLAY_WARNING=true DISPLAY_WARNING=true start-storybook -p 9009 --no-open", "build:storybook": "cross-env STORYBOOK_DISPLAY_WARNING=true DISPLAY_WARNING=true build-storybook -o ./storybook", "build": "rm -rf dist && yarn run build:prod", "build:dev": "cross-env NODE_ENV=development rollup -c", @@ -82,6 +82,7 @@ "@storybook/react": "6.5.10", "@testing-library/jest-dom": "^5.16.4", "@testing-library/react": "^12.1.5", + "@testing-library/react-hooks": "^8.0.1", "@types/jest": "^28.1.6", "@types/react": "^18.0.15", "@types/react-dom": "^18.0.6", diff --git a/src/DatePicker/DatePicker.tsx b/src/DatePicker/DatePicker.tsx index a8aaaf10..11d1c53d 100644 --- a/src/DatePicker/DatePicker.tsx +++ b/src/DatePicker/DatePicker.tsx @@ -5,7 +5,6 @@ import { Button } from '../Button/Button'; import { NumberInput } from '../NumberInput/NumberInput'; import { ScrollView } from '../ScrollView/ScrollView'; import { Select } from '../Select/Select'; -import { SelectChangeEvent } from '../Select/Select.types'; import { Toolbar } from '../Toolbar/Toolbar'; import { Window, WindowContent, WindowHeader } from '../Window/Window'; @@ -101,9 +100,12 @@ const DatePicker = forwardRef( const [date, setDate] = useState(() => convertDateToState(initialDate)); const { year, month, day } = date; - const handleMonthSelect = useCallback((e: SelectChangeEvent) => { - setDate(currentDate => ({ ...currentDate, month: e.target.value })); - }, []); + const handleMonthSelect = useCallback( + ({ value: monthSelected }: { value: number }) => { + setDate(currentDate => ({ ...currentDate, month: monthSelected })); + }, + [] + ); const handleYearSelect = useCallback((yearSelected: number) => { setDate(currentDate => ({ ...currentDate, year: yearSelected })); diff --git a/src/Select/Select.spec.tsx b/src/Select/Select.spec.tsx index 6a77559c..01f53842 100644 --- a/src/Select/Select.spec.tsx +++ b/src/Select/Select.spec.tsx @@ -1,8 +1,7 @@ -// Bsased on https://github.com/mui-org/material-ui - -import { fireEvent } from '@testing-library/react'; +import { fireEvent, screen, waitFor } from '@testing-library/react'; import React from 'react'; import { renderWithTheme } from '../../test/utils'; +import { noOp } from '../common/utils'; import { Select } from './Select'; import { SelectOption, SelectRef } from './Select.types'; @@ -15,7 +14,7 @@ const options: SelectOption[] = [ describe(' + ', () => { }); it('renders dropdown button with icon', () => { - const { getByTestId } = renderWithTheme( - ); - const button = getByTestId('select-button'); + const button = screen.getByTestId('select-button'); expect(button).toBeInTheDocument(); // we render styled.button, but as='div' // because it's used only for aesthetic purposes @@ -36,28 +33,49 @@ describe(' - ); - expect(getByRole('button')).toHaveProperty('tabIndex', 1); + renderWithTheme(); + renderWithTheme( + ); - expect(container.querySelector('input')).toHaveAttribute('type', 'hidden'); + + const trigger = screen.getByRole('button'); + fireEvent.focus(trigger); + fireEvent.blur(trigger); + expect(handleBlur).toHaveBeenCalledTimes(1); }); + it('should ignore onBlur when the menu opens', () => { // mousedown calls focus while click opens moving the focus to an item // this means the trigger is blurred immediately const handleBlur = jest.fn(); - const { getByRole, getAllByRole, queryByRole } = renderWithTheme( + renderWithTheme( ', () => { ]} /> ); - const trigger = getByRole('button'); + const trigger = screen.getByRole('button'); fireEvent.mouseDown(trigger); expect(handleBlur).toHaveBeenCalledTimes(0); - expect(getByRole('listbox')).toBeInTheDocument(); - const o = getAllByRole('option'); + expect(screen.getByRole('listbox')).toBeInTheDocument(); + const o = screen.getAllByRole('option'); fireEvent.mouseDown(o[0]); o[0].click(); expect(handleBlur).toHaveBeenCalledTimes(0); - expect(queryByRole('listbox', { hidden: false })).toBe(null); + expect(screen.queryByRole('listbox', { hidden: false })).toBe(null); }); + it('options should have a data-value attribute', () => { - const { getAllByRole } = renderWithTheme( - ); - const o = getAllByRole('option'); + const o = screen.getAllByRole('option'); expect(o[0]).toHaveAttribute('data-value', '10'); expect(o[1]).toHaveAttribute('data-value', '20'); }); - [' ', 'ArrowUp', 'ArrowDown', 'Enter'].forEach(key => { - it(`should open menu when pressed ${ - key === ' ' ? 'Space' : key - } key on select`, () => { - const { getByRole } = renderWithTheme( - + + it('should call onClose when user clicks outside of component', async () => { + const handleClose = jest.fn(); + renderWithTheme( +
+ - //
swag
- //
- // ); - // expect(handleClose).toHaveBeenCalledTimes(0); - // act(() => { - // fireEvent.click(getByTestId('el')); - // }); - // expect(handleClose).toHaveBeenCalledTimes(1); - // }); + it('should open and close on mouseDown', async () => { + renderWithTheme( + ); + expect(screen.getByTestId('SelectInput')).toHaveProperty( + 'tagName', + 'INPUT' + ); + }); + }); describe('prop: menuMaxHeight', () => { it('sets max-height to dropdown', () => { - const { getByRole } = renderWithTheme( - ); - const listbox = getByRole('listbox') as HTMLElement; + const listbox = screen.getByRole('listbox') as HTMLElement; expect( listbox.getAttribute('style')?.includes('max-height: 220px') ).toBeTruthy(); }); }); + describe('prop: onClose, onFocus, onKeyDown, onOpen', () => { + it('passes event through', () => { + const handler = jest.fn(); + + renderWithTheme( + <> + + renderWithTheme( + - ); - const o = getAllByRole('option'); - expect(o[0]).not.toHaveAttribute('aria-selected'); - expect(o[1]).toHaveAttribute('aria-selected', 'true'); - expect(o[2]).not.toHaveAttribute('aria-selected'); + + describe('prop: readOnly', () => { + it('should not trigger any event with readOnly', () => { + renderWithTheme( + }); + + describe('prop: value', () => { + it('should select the option based on the value', () => { + renderWithTheme( + ); - const o = getAllByRole('option'); + const o = screen.getAllByRole('option'); expect(o[0]).toHaveAttribute('aria-selected', 'true'); expect(o[1]).not.toHaveAttribute('aria-selected'); }); + it('should be able to use an object', () => { const value = {}; - const { getByRole } = renderWithTheme( + renderWithTheme( ); + expect(screen.getByRole('listbox')).toBeInTheDocument(); + }); + + it('open only with the left mouse button click', () => { + // Right/middle mouse click shouldn't open the Select + renderWithTheme( + ); + expect(screen.getByRole('button')).toHaveTextContent('0b10'); + }); + }); + + describe('prop: ref', () => { + it('should be able to return the input node via a ref object', () => { + const ref = React.createRef(); + renderWithTheme(); + ref.current?.focus(); + expect(screen.getByRole('button')).toHaveFocus(); + }); + }); + + describe('spread props', () => { + it('should apply additional props to trigger element', () => { + renderWithTheme( + + ); + screen.getByRole('button').focus(); + const focusedButton = document.activeElement as HTMLButtonElement; + fireEvent.keyDown(focusedButton, { code }); + expect( + screen.getByRole('listbox', { hidden: false }) + ).toBeInTheDocument(); + fireEvent.keyUp(focusedButton, { code }); + expect( + screen.getByRole('listbox', { hidden: false }) + ).toBeInTheDocument(); + } + ); + + it('closes menu when pressing Escape', async () => { + const onClose = jest.fn(); + renderWithTheme( + + + ); + const button = screen.getByRole('button'); + fireEvent.mouseDown(button); + + const listbox = screen.getByRole('listbox'); + + expect(screen.getByRole('option', { name: 'ten' })).toBeInTheDocument(); + + fireEvent.keyDown(listbox, { code: 'ArrowDown' }); + expect(screen.getByRole('option', { name: 'twenty' })).toHaveFocus(); + + fireEvent.keyDown(listbox, { code: keyCode }); + + await waitFor(() => { + expect(onClose).toHaveBeenCalled(); + }); + expect(listbox).not.toBeInTheDocument(); + + expect(button).toHaveFocus(); + expect(onKeyDown).toHaveBeenCalledWith( + expect.objectContaining({ defaultPrevented: true }) + ); + } + ); + + it('passes through Enter, Escape, Tab and Shift + Tab when closed', () => { + const onKeyDown = jest.fn(); + renderWithTheme( + // eslint-disable-next-line jsx-a11y/no-static-element-interactions +
+ +
+ ); + + const button = screen.getByRole('button'); + button.focus(); + + const eventOptions = [ + { altKey: true }, + { ctrlKey: true }, + { metaKey: true }, + { shiftKey: true } + ]; + eventOptions.forEach(eventOption => { + fireEvent.keyDown(button, { ...eventOption, code: 'KeyT' }); + expect(button).toHaveTextContent('ten'); + expect(onKeyDown).toHaveBeenCalledWith( + expect.objectContaining({ defaultPrevented: false }) + ); + }); + }); + + it('moves options using ArrowUp, ArrowDown, Home and End', async () => { + renderWithTheme(); + const button = screen.getByRole('button'); + fireEvent.mouseDown(button); + + const listbox = screen.getByRole('listbox'); + + expect(screen.getByRole('option', { name: 'ten' })).toBeInTheDocument(); + + fireEvent.keyDown(listbox, { code: 'KeyT' }); + expect(screen.getByRole('option', { name: 'ten' })).toHaveFocus(); + + fireEvent.keyDown(listbox, { code: 'KeyT' }); + expect(screen.getByRole('option', { name: 'twenty' })).toHaveFocus(); + + fireEvent.keyDown(listbox, { code: 'KeyT' }); + expect(screen.getByRole('option', { name: 'thirty' })).toHaveFocus(); + + fireEvent.keyDown(listbox, { code: 'KeyT' }); + expect(screen.getByRole('option', { name: 'ten' })).toHaveFocus(); + }); + + it('cycles through options when pressing the same key (closed menu)', async () => { + renderWithTheme(); + const button = screen.getByRole('button'); + fireEvent.mouseDown(button); + + const listbox = screen.getByRole('listbox'); + + expect(screen.getByRole('option', { name: 'ten' })).toBeInTheDocument(); + + fireEvent.keyDown(listbox, { code: 'KeyT' }); + expect(screen.getByRole('option', { name: 'ten' })).toHaveFocus(); + + fireEvent.keyDown(listbox, { code: 'KeyT' }); + expect(screen.getByRole('option', { name: 'twenty' })).toHaveFocus(); + + fireEvent.keyDown(listbox, { code: 'KeyE' }); + expect(screen.getByRole('option', { name: 'ten' })).toHaveFocus(); + }); + + it('switches to cycling after search', async () => { + renderWithTheme(); + const button = screen.getByRole('button'); + fireEvent.mouseDown(button); + + const listbox = screen.getByRole('listbox'); + + expect(screen.getByRole('option', { name: 'ten' })).toBeInTheDocument(); + + fireEvent.keyDown(listbox, { code: 'KeyT' }); + expect(screen.getByRole('option', { name: 'ten' })).toHaveFocus(); + + fireEvent.keyDown(listbox, { code: 'KeyH' }); + fireEvent.keyDown(listbox, { code: 'KeyI' }); + fireEvent.keyDown(listbox, { code: 'KeyR' }); + expect(screen.getByRole('option', { name: 'thirty' })).toHaveFocus(); + }); + + it('resets typing after timeout', async () => { + jest.useFakeTimers(); + renderWithTheme(', () => { it('sets aria-expanded="true" when the listbox is displayed', () => { // since we make the rest of the UI inaccessible when open this doesn't // technically matter. This is only here in case we keep the rest accessible - const { getByRole } = renderWithTheme(); + expect(screen.getByRole('button', { hidden: true })).toHaveAttribute( 'aria-expanded', 'true' ); }); - it('aria-expanded is not present if the listbox isnt displayed', () => { - const { getByRole } = renderWithTheme(); + expect(screen.getByRole('button')).toHaveAttribute( + 'aria-expanded', + 'false' + ); }); + it('indicates that activating the button displays a listbox', () => { - const { getByRole } = renderWithTheme(); + expect(screen.getByRole('button')).toHaveAttribute( + 'aria-haspopup', + 'listbox' + ); }); + it('renders an element with listbox behavior', () => { - const { getByRole } = renderWithTheme(); + expect(screen.getByRole('listbox')).toBeVisible(); }); it('the listbox is focusable', () => { - const { getByRole } = renderWithTheme(); + const listbox = screen.getByRole('listbox'); listbox.focus(); expect(listbox).toHaveFocus(); }); + it('identifies each selectable element containing an option', () => { - const { getAllByRole } = renderWithTheme( - ); + const o = screen.getAllByRole('option'); expect(o[0]).toHaveTextContent('ten'); expect(o[1]).toHaveTextContent('twenty'); }); + it('indicates the selected option', () => { - const { getAllByRole } = renderWithTheme( - ); - expect(getAllByRole('option')[1]).toHaveAttribute( + expect(screen.getAllByRole('option')[1]).toHaveAttribute( 'aria-selected', 'true' ); }); + it('it will fallback to its content for the accessible name when it has no name', () => { - const { getByRole } = renderWithTheme(); - const button = getByRole('button'); - expect(button).toHaveAttribute( - 'aria-labelledby', - button.getAttribute('id') - ); + renderWithTheme( Chose second option: - - ); - expect(getByRole('button')).toHaveAttribute( - 'aria-labelledby', - `select-label ${getByRole('button').getAttribute('id')}` - ); - }); - it('the list of options is not labelled by default', () => { - const { getByRole } = renderWithTheme( - - ); - expect(getByRole('listbox')).toHaveAttribute( - 'aria-labelledby', - 'select-label' - ); - }); - }); - describe('prop: readOnly', () => { - it('should not trigger any event with readOnly', () => { - const { getByRole, queryByRole } = renderWithTheme( - - ); - expect(getByRole('button')).toHaveAttribute('data-test', 'SelectDisplay'); - }); - }); - - describe('prop: renderValue', () => { - it('should use the prop to render the value', () => { - const formatDisplay = (x: SelectOption) => - `0b${Number(x.value).toString(2)}`; - const { getByRole } = renderWithTheme( - - ); - expect(getByRole('listbox')).toBeInTheDocument(); - }); - it('open only with the left mouse button click', () => { - // Right/middle mouse click shouldn't open the Select - const { getByRole, queryByRole } = renderWithTheme( - ); - expect(ref.current?.node).toHaveProperty('tagName', 'INPUT'); - }); - - it('should be able focus the trigger imperatively', () => { - const ref = React.createRef(); - const { getByRole } = renderWithTheme(); - expect(getByRole('button')).not.toHaveAttribute('id'); - }); - it('should have select-`name` id when name is provided', () => { - const { getByRole } = renderWithTheme(', () => { - const { container } = renderWithTheme( - - ); - expect(getByLabelText('A select')).toHaveProperty('tagName', 'SELECT'); + const triggers = screen.getAllByRole('button'); + expect(triggers[0]).toHaveAttribute('aria-labelledby', 'select-1-label'); + expect(triggers[1]).toHaveAttribute('aria-labelledby', 'select-2-label'); }); }); }); diff --git a/src/Select/Select.stories.data.ts b/src/Select/Select.stories.data.ts new file mode 100644 index 00000000..000a7a19 --- /dev/null +++ b/src/Select/Select.stories.data.ts @@ -0,0 +1,153 @@ +export const PokemonOptions = [ + 'Bulbasaur', + 'Ivysaur', + 'Venusaur', + 'Charmander', + 'Charmeleon', + 'Charizard', + 'Squirtle', + 'Wartortle', + 'Blastoise', + 'Caterpie', + 'Metapod', + 'Butterfree', + 'Weedle', + 'Kakuna', + 'Beedrill', + 'Pidgey', + 'Pidgeotto', + 'Pidgeot', + 'Rattata', + 'Raticate', + 'Spearow', + 'Fearow', + 'Ekans', + 'Arbok', + 'Pikachu', + 'Raichu', + 'Sandshrew', + 'Sandslash', + 'Nidoran♀', + 'Nidorina', + 'Nidoqueen', + 'Nidoran♂', + 'Nidorino', + 'Nidoking', + 'Clefairy', + 'Clefable', + 'Vulpix', + 'Ninetales', + 'Jigglypuff', + 'Wigglytuff', + 'Zubat', + 'Golbat', + 'Oddish', + 'Gloom', + 'Vileplume', + 'Paras', + 'Parasect', + 'Venonat', + 'Venomoth', + 'Diglett', + 'Dugtrio', + 'Meowth', + 'Persian', + 'Psyduck', + 'Golduck', + 'Mankey', + 'Primeape', + 'Growlithe', + 'Arcanine', + 'Poliwag', + 'Poliwhirl', + 'Poliwrath', + 'Abra', + 'Kadabra', + 'Alakazam', + 'Machop', + 'Machoke', + 'Machamp', + 'Bellsprout', + 'Weepinbell', + 'Victreebel', + 'Tentacool', + 'Tentacruel', + 'Geodude', + 'Graveler', + 'Golem', + 'Ponyta', + 'Rapidash', + 'Slowpoke', + 'Slowbro', + 'Magnemite', + 'Magneton', + 'Farfetch’d', + 'Doduo', + 'Dodrio', + 'Seel', + 'Dewgong', + 'Grimer', + 'Muk', + 'Shellder', + 'Cloyster', + 'Gastly', + 'Haunter', + 'Gengar', + 'Onix', + 'Drowzee', + 'Hypno', + 'Krabby', + 'Kingler', + 'Voltorb', + 'Electrode', + 'Exeggcute', + 'Exeggutor', + 'Cubone', + 'Marowak', + 'Hitmonlee', + 'Hitmonchan', + 'Lickitung', + 'Koffing', + 'Weezing', + 'Rhyhorn', + 'Rhydon', + 'Chansey', + 'Tangela', + 'Kangaskhan', + 'Horsea', + 'Seadra', + 'Goldeen', + 'Seaking', + 'Staryu', + 'Starmie', + 'Mr. Mime', + 'Scyther', + 'Jynx', + 'Electabuzz', + 'Magmar', + 'Pinsir', + 'Tauros', + 'Magikarp', + 'Gyarados', + 'Lapras', + 'Ditto', + 'Eevee', + 'Vaporeon', + 'Jolteon', + 'Flareon', + 'Porygon', + 'Omanyte', + 'Omastar', + 'Kabuto', + 'Kabutops', + 'Aerodactyl', + 'Snorlax', + 'Articuno', + 'Zapdos', + 'Moltres', + 'Dratini', + 'Dragonair', + 'Dragonite', + 'Mewtwo', + 'Mew' +].map((label, index) => ({ value: index + 1, label })); diff --git a/src/Select/Select.stories.tsx b/src/Select/Select.stories.tsx index a0621276..b3aa769c 100644 --- a/src/Select/Select.stories.tsx +++ b/src/Select/Select.stories.tsx @@ -2,19 +2,24 @@ import { ComponentMeta } from '@storybook/react'; import React from 'react'; -import { GroupBox, ScrollView, Select, Window, WindowContent } from 'react95'; +import { + GroupBox, + ScrollView, + Select, + SelectNative, + Window, + WindowContent +} from 'react95'; import styled from 'styled-components'; -import { SelectChangeEvent, SelectOption } from './Select.types'; +import { PokemonOptions } from './Select.stories.data'; +import { SelectOption } from './Select.types'; -const options = [ - { value: 1, label: '⚡ Pikachu' }, - { value: 2, label: '🌿 Bulbasaur' }, - { value: 3, label: '💦 Squirtle' }, - { value: 4, label: '🔥 Mega Charizard Y' }, - { value: 5, label: '🎤 Jigglypuff' }, - { value: 6, label: '🛌🏻 Snorlax' }, - { value: 7, label: '⛰ Geodude' } -]; +const options = PokemonOptions; + +const nativeOptions = options.map(option => ({ + ...option, + value: String(option.value) +})); const Wrapper = styled.div` background: ${({ theme }) => theme.material}; @@ -39,8 +44,10 @@ const Wrapper = styled.div` } `; -const onChange = (event: SelectChangeEvent, option: SelectOption) => - console.log(event, option); +const onChange = ( + selectedOption: SelectOption, + changeOptions: { fromEvent: React.SyntheticEvent | Event } +) => console.log(selectedOption, changeOptions.fromEvent); export default { title: 'Controls/Select', @@ -73,25 +80,20 @@ export function Default() { /> - @@ -128,20 +130,18 @@ export function Flat() { /> - @@ -157,7 +157,7 @@ Flat.story = { export function CustomDisplayFormatting() { return ( ', () => { + const { container } = renderWithTheme(); + expect(container.querySelector('select')).toBeInTheDocument(); + }); + + it('renders uses values for labels', () => { + const optionsWithoutLabels = options.map(({ value }) => ({ value })); + renderWithTheme( + + ); + expect(screen.getByTestId('select')).toHaveTextContent('10'); + }); + + it('calls onChange if not disabled or readOnly', () => { + const handleChange = jest.fn(); + renderWithTheme( + <> + + + + + ); + fireEvent.change(screen.getByTestId('selectEnabled')); + fireEvent.change(screen.getByTestId('selectDisabled')); + fireEvent.change(screen.getByTestId('selectReadOnly')); + expect(handleChange).toHaveBeenCalledTimes(1); + expect(handleChange).toHaveBeenCalledWith(options[0], { + fromEvent: expect.objectContaining({ type: 'change' }) + }); + }); + + it('can be labelled with a