From ed3916f4d224ac86ff9361481478abcdc282650c Mon Sep 17 00:00:00 2001 From: Genevieve Luyt <11131143+genevieveluyt@users.noreply.github.com> Date: Wed, 15 Apr 2020 00:33:21 -0400 Subject: [PATCH 01/10] Fix auto id generation for combobox --- .../components/ComboBox/ComboBox.tsx | 734 ++++++++---------- .../ComboBox/tests/ComboBox.test.tsx | 39 +- 2 files changed, 351 insertions(+), 422 deletions(-) diff --git a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx index d0a40f6f123..87262894a03 100644 --- a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx +++ b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx @@ -1,6 +1,6 @@ -import React from 'react'; -import {createUniqueIDFactory} from '@shopify/javascript-utilities/other'; +import React, {useState, useEffect, useCallback} from 'react'; +import {useUniqueId} from '../../../../utilities/unique-id'; import {OptionList, OptionDescriptor} from '../../../OptionList'; import {ActionList} from '../../../ActionList'; import {Popover, PopoverProps} from '../../../Popover'; @@ -11,18 +11,6 @@ import {EventListener} from '../../../EventListener'; import {ComboBoxContext} from './context'; import styles from './ComboBox.scss'; -const getUniqueId = createUniqueIDFactory('ComboBox'); - -interface State { - comboBoxId: string; - selectedOption?: OptionDescriptor | ActionListItemDescriptor | undefined; - selectedIndex: number; - selectedOptions: string[]; - navigableOptions: (OptionDescriptor | ActionListItemDescriptor)[]; - popoverActive: boolean; - popoverWasActive: boolean; -} - export interface ComboBoxProps { /** A unique identifier for the ComboBox */ id?: string; @@ -53,427 +41,374 @@ export interface ComboBoxProps { /** Callback when the end of the list is reached */ onEndReached?(): void; } +export function ComboBox({ + id, + options, + selected, + textField, + preferredPosition, + listTitle, + allowMultiple, + actionsBefore, + actionsAfter, + contentBefore, + contentAfter, + emptyState, + onSelect, + onEndReached, +}: ComboBoxProps) { + const [comboBoxId, setComboBoxId] = useState( + useUniqueId('ComboBox', id), + ); + const [selectedOption, setSelectedOption] = useState< + OptionDescriptor | ActionListItemDescriptor | undefined + >(undefined); + const [selectedIndex, setSelectedIndex] = useState(-1); + const [selectedOptions, setSelectedOptions] = useState(selected); + const [navigableOptions, setNavigableOptions] = useState< + (OptionDescriptor | ActionListItemDescriptor)[] + >([]); + const [popoverActive, setPopoverActive] = useState(false); + const [popoverWasActive, setPopoverWasActive] = useState(false); + + const getActionsWithIds = useCallback( + ( + actions: ActionListItemDescriptor[], + before?: boolean, + ): ActionListItemDescriptor[] => { + if (before) { + return navigableOptions.slice(0, actions.length); + } else { + return navigableOptions.slice(-actions.length); + } + }, + [navigableOptions], + ); -export class ComboBox extends React.PureComponent { - static getDerivedStateFromProps( - { - options: nextOptions, - selected: nextSelected, - actionsBefore: nextActionsBefore, - actionsAfter: nextActionsAfter, - }: ComboBoxProps, - {navigableOptions, selectedOptions, comboBoxId}: State, - ) { - const optionsChanged = - filterForOptions(navigableOptions) && - nextOptions && - !optionsAreEqual(navigableOptions, nextOptions); - - let newNavigableOptions: ( - | OptionDescriptor - | ActionListItemDescriptor - )[] = []; - if (nextActionsBefore) { - newNavigableOptions = newNavigableOptions.concat(nextActionsBefore); - } - if (optionsChanged || nextActionsBefore) { - newNavigableOptions = newNavigableOptions.concat(nextOptions); - } - if (nextActionsAfter) { - newNavigableOptions = newNavigableOptions.concat(nextActionsAfter); - } - newNavigableOptions = assignOptionIds(newNavigableOptions, comboBoxId); + const handlePopoverClose = useCallback(() => { + setPopoverActive(false); + setPopoverWasActive(false); + }, []); - if (optionsChanged && selectedOptions !== nextSelected) { - return { - navigableOptions: newNavigableOptions, - selectedOptions: nextSelected, - }; - } else if (optionsChanged) { - return { - navigableOptions: newNavigableOptions, - }; - } else if (selectedOptions !== nextSelected) { - return {selectedOptions: nextSelected}; + const handlePopoverOpen = useCallback(() => { + if (!popoverActive && navigableOptions && navigableOptions.length > 0) { + setPopoverActive(true); + setPopoverWasActive(true); } - return null; - } - - state: State = { - comboBoxId: this.getComboBoxId(), - selectedOption: undefined, - selectedIndex: -1, - selectedOptions: this.props.selected, - navigableOptions: [], - popoverActive: false, - popoverWasActive: false, - }; + }, [navigableOptions, popoverActive]); - componentDidMount() { - const {options, actionsBefore, actionsAfter} = this.props; - const comboBoxId = this.getComboBoxId(); - let navigableOptions: (OptionDescriptor | ActionListItemDescriptor)[] = []; - - if (actionsBefore) { - navigableOptions = navigableOptions.concat(actionsBefore); - } - if (options) { - navigableOptions = navigableOptions.concat(options); - } - if (actionsAfter) { - navigableOptions = navigableOptions.concat(actionsAfter); - } - navigableOptions = assignOptionIds(navigableOptions, comboBoxId); + const visuallyUpdateSelectedOption = useCallback( + ( + newOption: OptionDescriptor | ActionListItemDescriptor, + oldOption: OptionDescriptor | ActionListItemDescriptor | undefined, + ) => { + if (oldOption) { + oldOption.active = false; + } + if (newOption) { + newOption.active = true; + } + }, + [], + ); - this.setState({ - navigableOptions, - }); - } + const resetVisuallySelectedOptions = useCallback(() => { + setSelectedOption(undefined); + setSelectedIndex(-1); + navigableOptions && + navigableOptions.forEach( + (option: OptionDescriptor | ActionListItemDescriptor) => { + option.active = false; + }, + ); + }, [navigableOptions]); - componentDidUpdate(_: ComboBoxProps, prevState: State) { - const {contentBefore, contentAfter, emptyState} = this.props; - const {navigableOptions, popoverWasActive} = this.state; + const selectOptionAtIndex = useCallback( + (newOptionIndex: number) => { + if (!navigableOptions || navigableOptions.length === 0) { + return; + } - const optionsChanged = - navigableOptions && - prevState.navigableOptions && - !optionsAreEqual(navigableOptions, prevState.navigableOptions); + const newSelectedOption = navigableOptions[newOptionIndex]; - if (optionsChanged) { - this.updateIndexOfSelectedOption(navigableOptions); - } + visuallyUpdateSelectedOption(newSelectedOption, selectedOption); - if ( - navigableOptions && - navigableOptions.length === 0 && - !contentBefore && - !contentAfter && - !emptyState - ) { - // eslint-disable-next-line react/no-did-update-set-state - this.setState({popoverActive: false}); - } else if ( - popoverWasActive && - navigableOptions && - navigableOptions.length !== 0 - ) { - // eslint-disable-next-line react/no-did-update-set-state - this.setState({popoverActive: true}); - } - } + setSelectedOption(newSelectedOption); + setSelectedIndex(newOptionIndex); + }, + [navigableOptions, selectedOption, visuallyUpdateSelectedOption], + ); - getComboBoxId(): string { - if (this.state && this.state.comboBoxId) { - return this.state.comboBoxId; + const selectNextOption = useCallback(() => { + if (!navigableOptions || navigableOptions.length === 0) { + return; } - return this.props.id || getUniqueId(); - } - getActionsWithIds( - actions: ActionListItemDescriptor[], - before?: boolean, - ): ActionListItemDescriptor[] { - const {navigableOptions} = this.state; + let newIndex = selectedIndex; - if (before) { - return navigableOptions.slice(0, actions.length); + if (selectedIndex + 1 >= navigableOptions.length) { + newIndex = 0; } else { - return navigableOptions.slice(-actions.length); + newIndex++; } - } - render() { - const { - options, - textField, - listTitle, - allowMultiple, - preferredPosition, - actionsBefore, - actionsAfter, - contentBefore, - contentAfter, - onEndReached, - emptyState, - } = this.props; - const {comboBoxId, navigableOptions, selectedOptions} = this.state; - - let actionsBeforeMarkup: JSX.Element | undefined; - if (actionsBefore && actionsBefore.length > 0) { - actionsBeforeMarkup = ( - - ); - } + selectOptionAtIndex(newIndex); + }, [navigableOptions, selectOptionAtIndex, selectedIndex]); - let actionsAfterMarkup: JSX.Element | undefined; - if (actionsAfter && actionsAfter.length > 0) { - actionsAfterMarkup = ( - - ); + const selectPreviousOption = useCallback(() => { + if (!navigableOptions || navigableOptions.length === 0) { + return; } - const optionsMarkup = options.length > 0 && ( - - ); - - const emptyStateMarkup = !actionsAfter && - !actionsBefore && - !contentAfter && - !contentBefore && - options.length === 0 && - emptyState &&
{emptyState}
; - - const context = { - comboBoxId, - selectedOptionId: this.selectedOptionId(), - }; - - return ( - -
- - - - - - -
- {contentBefore} - {actionsBeforeMarkup} - {optionsMarkup} - {actionsAfterMarkup} - {contentAfter} - {emptyStateMarkup} -
- - -
- - ); - } + let newIndex = selectedIndex; - private handleDownArrow = () => { - this.selectNextOption(); - this.handlePopoverOpen; - }; + if (selectedIndex <= 0) { + newIndex = navigableOptions.length - 1; + } else { + newIndex--; + } - private handleUpArrow = () => { - this.selectPreviousOption(); - this.handlePopoverOpen; - }; + selectOptionAtIndex(newIndex); + }, [navigableOptions, selectOptionAtIndex, selectedIndex]); - private handleEnter = (event: KeyboardEvent) => { - if (event.keyCode !== Key.Enter) { - return; - } + const selectOptions = useCallback( + (selected: string[]) => { + selected && onSelect(selected); + if (!allowMultiple) { + resetVisuallySelectedOptions(); + setPopoverActive(false); + setPopoverWasActive(false); + } + }, + [allowMultiple, onSelect, resetVisuallySelectedOptions], + ); - const {selectedOption} = this.state; - if (this.state.popoverActive && selectedOption) { - if (isOption(selectedOption)) { - event.preventDefault(); - this.handleSelection(selectedOption.value); + const handleSelection = useCallback( + (newSelected: string) => { + let newlySelectedOptions = selected; + if (selected.includes(newSelected)) { + newlySelectedOptions.splice( + newlySelectedOptions.indexOf(newSelected), + 1, + ); + } else if (allowMultiple) { + newlySelectedOptions.push(newSelected); } else { - selectedOption.onAction && selectedOption.onAction(); + newlySelectedOptions = [newSelected]; } - } - - this.handlePopoverOpen; - }; - - private handleFocus = () => { - this.setState({popoverActive: true, popoverWasActive: true}); - }; - private handleBlur = () => { - this.setState({popoverActive: false, popoverWasActive: false}, () => { - this.resetVisuallySelectedOptions(); - }); - }; - - private handleClick = () => { - !this.state.popoverActive && this.setState({popoverActive: true}); - }; - - private handleSelection = (newSelected: string) => { - const {selected, allowMultiple} = this.props; - let newlySelectedOptions = selected; - if (selected.includes(newSelected)) { - newlySelectedOptions.splice(newlySelectedOptions.indexOf(newSelected), 1); - } else if (allowMultiple) { - newlySelectedOptions.push(newSelected); - } else { - newlySelectedOptions = [newSelected]; - } - - this.selectOptions(newlySelectedOptions); - }; + selectOptions(newlySelectedOptions); + }, + [allowMultiple, selectOptions, selected], + ); - private selectOptions = (selected: string[]) => { - const {onSelect, allowMultiple} = this.props; - selected && onSelect(selected); - if (!allowMultiple) { - this.resetVisuallySelectedOptions(); - this.setState({popoverActive: false, popoverWasActive: false}); - } - }; + const handleDownArrow = useCallback(() => { + selectNextOption(); + handlePopoverOpen; + }, [handlePopoverOpen, selectNextOption]); - private updateIndexOfSelectedOption = ( - newOptions: (OptionDescriptor | ActionListItemDescriptor)[], - ) => { - const {selectedIndex, selectedOption} = this.state; - if (selectedOption && newOptions.includes(selectedOption)) { - this.selectOptionAtIndex(newOptions.indexOf(selectedOption)); - } else if (selectedIndex > newOptions.length - 1) { - this.resetVisuallySelectedOptions(); - } else { - this.selectOptionAtIndex(selectedIndex); - } - }; + const handleUpArrow = useCallback(() => { + selectPreviousOption(); + handlePopoverOpen; + }, [handlePopoverOpen, selectPreviousOption]); - private resetVisuallySelectedOptions = () => { - const {navigableOptions} = this.state; - this.setState({ - selectedOption: undefined, - selectedIndex: -1, - }); - navigableOptions && - navigableOptions.forEach((option) => { - option.active = false; - }); - }; + const handleEnter = useCallback( + (event: KeyboardEvent) => { + if (event.keyCode !== Key.Enter) { + return; + } - private handlePopoverClose = () => { - this.setState({popoverActive: false, popoverWasActive: false}); - }; + if (popoverActive && selectedOption) { + if (isOption(selectedOption)) { + event.preventDefault(); + handleSelection(selectedOption.value); + } else { + selectedOption.onAction && selectedOption.onAction(); + } + } - private handlePopoverOpen = () => { - const {popoverActive, navigableOptions} = this.state; + handlePopoverOpen; + }, + [handlePopoverOpen, handleSelection, popoverActive, selectedOption], + ); - !popoverActive && - navigableOptions && - navigableOptions.length > 0 && - this.setState({popoverActive: true, popoverWasActive: true}); - }; + const handleFocus = useCallback(() => { + setPopoverActive(true); + setPopoverWasActive(true); + }, []); + + const handleBlur = useCallback(() => { + setPopoverActive(false); + setPopoverWasActive(false); + resetVisuallySelectedOptions(); + }, [resetVisuallySelectedOptions]); + + const handleClick = useCallback(() => { + !popoverActive && setPopoverActive(true); + }, [popoverActive]); + + const updateIndexOfSelectedOption = useCallback( + (newOptions: (OptionDescriptor | ActionListItemDescriptor)[]) => { + if (selectedOption && newOptions.includes(selectedOption)) { + selectOptionAtIndex(newOptions.indexOf(selectedOption)); + } else if (selectedIndex > newOptions.length - 1) { + resetVisuallySelectedOptions(); + } else { + selectOptionAtIndex(selectedIndex); + } + }, + [ + resetVisuallySelectedOptions, + selectOptionAtIndex, + selectedIndex, + selectedOption, + ], + ); - private selectNextOption = () => { - const {selectedIndex, navigableOptions} = this.state; + useEffect(() => { + id && setComboBoxId(id); + }, [id]); - if (!navigableOptions || navigableOptions.length === 0) { - return; + useEffect(() => { + if (selectedOptions !== selected) { + setSelectedOptions(selected); } + }, [selected, selectedOptions]); - let newIndex = selectedIndex; - - if (selectedIndex + 1 >= navigableOptions.length) { - newIndex = 0; - } else { - newIndex++; + useEffect(() => { + let newNavigableOptions: ( + | OptionDescriptor + | ActionListItemDescriptor + )[] = []; + if (actionsBefore) { + newNavigableOptions = newNavigableOptions.concat(actionsBefore); } - - this.selectOptionAtIndex(newIndex); - }; - - private selectPreviousOption = () => { - const {selectedIndex, navigableOptions} = this.state; - - if (!navigableOptions || navigableOptions.length === 0) { - return; + if (options) { + newNavigableOptions = newNavigableOptions.concat(options); + } + if (actionsAfter) { + newNavigableOptions = newNavigableOptions.concat(actionsAfter); } + newNavigableOptions = assignOptionIds(newNavigableOptions, comboBoxId); + setNavigableOptions(newNavigableOptions); + }, [actionsAfter, actionsBefore, comboBoxId, options]); - let newIndex = selectedIndex; + useEffect(() => { + updateIndexOfSelectedOption(navigableOptions); + }, [navigableOptions, updateIndexOfSelectedOption]); - if (selectedIndex <= 0) { - newIndex = navigableOptions.length - 1; - } else { - newIndex--; + useEffect(() => { + if ( + navigableOptions && + navigableOptions.length === 0 && + !contentBefore && + !contentAfter && + !emptyState + ) { + setPopoverActive(false); + } else if ( + popoverWasActive && + navigableOptions && + navigableOptions.length !== 0 + ) { + setPopoverActive(true); } + }, [ + contentAfter, + contentBefore, + emptyState, + navigableOptions, + popoverWasActive, + ]); + + let actionsBeforeMarkup: JSX.Element | undefined; + if (actionsBefore && actionsBefore.length > 0) { + actionsBeforeMarkup = ( + + ); + } - this.selectOptionAtIndex(newIndex); - }; + let actionsAfterMarkup: JSX.Element | undefined; + if (actionsAfter && actionsAfter.length > 0) { + actionsAfterMarkup = ( + + ); + } - private selectOptionAtIndex = (newOptionIndex: number) => { - this.setState((prevState) => { - if ( - !prevState.navigableOptions || - prevState.navigableOptions.length === 0 - ) { - return prevState; - } - const newSelectedOption = prevState.navigableOptions[newOptionIndex]; + const optionsMarkup = options.length > 0 && ( + + ); - this.visuallyUpdateSelectedOption( - newSelectedOption, - prevState.selectedOption, - ); + const emptyStateMarkup = !actionsAfter && + !actionsBefore && + !contentAfter && + !contentBefore && + options.length === 0 && + emptyState &&
{emptyState}
; - return { - ...prevState, - selectedOption: newSelectedOption, - selectedIndex: newOptionIndex, - }; - }); - }; + const selectedOptionId = selectedOption + ? `${comboBoxId}-${selectedIndex}` + : undefined; - private visuallyUpdateSelectedOption = ( - newOption: OptionDescriptor | ActionListItemDescriptor, - oldOption: OptionDescriptor | ActionListItemDescriptor | undefined, - ) => { - if (oldOption) { - oldOption.active = false; - } - if (newOption) { - newOption.active = true; - } + const context = { + comboBoxId, + selectedOptionId, }; - private selectedOptionId(): string | undefined { - const {selectedOption, selectedIndex, comboBoxId} = this.state; - return selectedOption ? `${comboBoxId}-${selectedIndex}` : undefined; - } + return ( + +
+ + + + + + +
+ {contentBefore} + {actionsBeforeMarkup} + {optionsMarkup} + {actionsAfterMarkup} + {contentAfter} + {emptyStateMarkup} +
+ + +
+ + ); } function assignOptionIds( @@ -491,31 +426,6 @@ function assignOptionIds( ); } -function optionsAreEqual( - firstOptions: (OptionDescriptor | ActionListItemDescriptor)[], - secondOptions: (OptionDescriptor | ActionListItemDescriptor)[], -) { - if (firstOptions.length !== secondOptions.length) { - return false; - } - return firstOptions.every( - (firstItem: OptionDescriptor | ActionListItemDescriptor, index: number) => { - const secondItem = secondOptions[index]; - if (isOption(firstItem)) { - if (isOption(secondItem)) { - return firstItem.value === secondItem.value; - } - return false; - } else { - if (!isOption(secondItem)) { - return firstItem.content === secondItem.content; - } - return false; - } - }, - ); -} - function isOption( navigableOption: OptionDescriptor | ActionListItemDescriptor, ): navigableOption is OptionDescriptor { diff --git a/src/components/Autocomplete/components/ComboBox/tests/ComboBox.test.tsx b/src/components/Autocomplete/components/ComboBox/tests/ComboBox.test.tsx index 822ade6a327..d6577157eb1 100644 --- a/src/components/Autocomplete/components/ComboBox/tests/ComboBox.test.tsx +++ b/src/components/Autocomplete/components/ComboBox/tests/ComboBox.test.tsx @@ -2,7 +2,7 @@ import React from 'react'; import {OptionList, ActionList, Popover} from 'components'; import {mountWithApp} from 'test-utilities'; // eslint-disable-next-line no-restricted-imports -import {mountWithAppProvider} from 'test-utilities/legacy'; +import {mountWithAppProvider, act} from 'test-utilities/legacy'; import {TextField} from '../../TextField'; import {Key} from '../../../../../types'; @@ -401,8 +401,12 @@ describe('', () => { />, ); comboBox.find(TextField).simulate('click'); - dispatchKeyup(Key.DownArrow); - dispatchKeydown(Key.Enter); + act(() => { + dispatchKeyup(Key.DownArrow); + }); + act(() => { + dispatchKeydown(Key.Enter); + }); expect(spy).not.toHaveBeenCalled(); comboBox.setProps({ @@ -414,8 +418,12 @@ describe('', () => { }); comboBox.update(); comboBox.find(TextField).simulate('click'); - dispatchKeyup(Key.DownArrow); - dispatchKeydown(Key.Enter); + act(() => { + dispatchKeyup(Key.DownArrow); + }); + act(() => { + dispatchKeydown(Key.Enter); + }); expect(spy).toHaveBeenCalledWith(['cheese_pizza']); }); @@ -430,8 +438,12 @@ describe('', () => { />, ); comboBox.find(TextField).simulate('click'); - dispatchKeyup(Key.DownArrow); - dispatchKeydown(Key.Enter); + act(() => { + dispatchKeyup(Key.DownArrow); + }); + act(() => { + dispatchKeydown(Key.Enter); + }); expect(spy).toHaveBeenCalledWith(['cheese_pizza']); }); @@ -446,8 +458,12 @@ describe('', () => { />, ); comboBox.find(TextField).simulate('click'); - dispatchKeyup(Key.DownArrow); - dispatchKeydown(Key.RightArrow); + act(() => { + dispatchKeyup(Key.DownArrow); + }); + act(() => { + dispatchKeydown(Key.RightArrow); + }); expect(spy).not.toHaveBeenCalled(); }); @@ -478,7 +494,10 @@ describe('', () => { comboBox.find(TextField).simulate('click'); expect(comboBox.find(Popover).prop('active')).toBe(true); - dispatchKeyup(Key.Escape); + act(() => { + dispatchKeyup(Key.Escape); + }); + comboBox.update(); expect(comboBox.find(Popover).prop('active')).toBe(false); }); From e0a2077cd88a13790f5bcfc1f9873d92554753d2 Mon Sep 17 00:00:00 2001 From: Genevieve Luyt <11131143+genevieveluyt@users.noreply.github.com> Date: Wed, 15 Apr 2020 15:14:27 -0400 Subject: [PATCH 02/10] Don't use state for id --- .../components/ComboBox/ComboBox.tsx | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx index 87262894a03..53d22ab772a 100644 --- a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx +++ b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx @@ -42,7 +42,7 @@ export interface ComboBoxProps { onEndReached?(): void; } export function ComboBox({ - id, + id: idProp, options, selected, textField, @@ -57,9 +57,6 @@ export function ComboBox({ onSelect, onEndReached, }: ComboBoxProps) { - const [comboBoxId, setComboBoxId] = useState( - useUniqueId('ComboBox', id), - ); const [selectedOption, setSelectedOption] = useState< OptionDescriptor | ActionListItemDescriptor | undefined >(undefined); @@ -71,6 +68,8 @@ export function ComboBox({ const [popoverActive, setPopoverActive] = useState(false); const [popoverWasActive, setPopoverWasActive] = useState(false); + const id = useUniqueId('ComboBox', idProp); + const getActionsWithIds = useCallback( ( actions: ActionListItemDescriptor[], @@ -265,10 +264,6 @@ export function ComboBox({ ], ); - useEffect(() => { - id && setComboBoxId(id); - }, [id]); - useEffect(() => { if (selectedOptions !== selected) { setSelectedOptions(selected); @@ -289,9 +284,9 @@ export function ComboBox({ if (actionsAfter) { newNavigableOptions = newNavigableOptions.concat(actionsAfter); } - newNavigableOptions = assignOptionIds(newNavigableOptions, comboBoxId); + newNavigableOptions = assignOptionIds(newNavigableOptions, id); setNavigableOptions(newNavigableOptions); - }, [actionsAfter, actionsBefore, comboBoxId, options]); + }, [actionsAfter, actionsBefore, id, options]); useEffect(() => { updateIndexOfSelectedOption(navigableOptions); @@ -358,11 +353,11 @@ export function ComboBox({ emptyState &&
{emptyState}
; const selectedOptionId = selectedOption - ? `${comboBoxId}-${selectedIndex}` + ? `${id}-${selectedIndex}` : undefined; const context = { - comboBoxId, + id, selectedOptionId, }; @@ -372,8 +367,8 @@ export function ComboBox({ onClick={handleClick} role="combobox" aria-expanded={popoverActive} - aria-owns={comboBoxId} - aria-controls={comboBoxId} + aria-owns={id} + aria-controls={id} aria-haspopup onFocus={handleFocus} onBlur={handleBlur} @@ -392,11 +387,7 @@ export function ComboBox({ preventAutofocus > -
+
{contentBefore} {actionsBeforeMarkup} {optionsMarkup} @@ -413,7 +404,7 @@ export function ComboBox({ function assignOptionIds( options: (OptionDescriptor | ActionListItemDescriptor)[], - comboBoxId: string, + id: string, ): OptionDescriptor[] | ActionListItemDescriptor[] { return options.map( ( @@ -421,7 +412,7 @@ function assignOptionIds( optionIndex: number, ) => ({ ...option, - id: `${comboBoxId}-${optionIndex}`, + id: `${id}-${optionIndex}`, }), ); } From de1ac6931bb3910e436c67692416312af955d811 Mon Sep 17 00:00:00 2001 From: Genevieve Luyt <11131143+genevieveluyt@users.noreply.github.com> Date: Wed, 15 Apr 2020 15:15:43 -0400 Subject: [PATCH 03/10] Use type inference for state --- .../Autocomplete/components/ComboBox/ComboBox.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx index 53d22ab772a..9dff6fd8f72 100644 --- a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx +++ b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx @@ -60,13 +60,13 @@ export function ComboBox({ const [selectedOption, setSelectedOption] = useState< OptionDescriptor | ActionListItemDescriptor | undefined >(undefined); - const [selectedIndex, setSelectedIndex] = useState(-1); - const [selectedOptions, setSelectedOptions] = useState(selected); + const [selectedIndex, setSelectedIndex] = useState(-1); + const [selectedOptions, setSelectedOptions] = useState(selected); const [navigableOptions, setNavigableOptions] = useState< (OptionDescriptor | ActionListItemDescriptor)[] >([]); - const [popoverActive, setPopoverActive] = useState(false); - const [popoverWasActive, setPopoverWasActive] = useState(false); + const [popoverActive, setPopoverActive] = useState(false); + const [popoverWasActive, setPopoverWasActive] = useState(false); const id = useUniqueId('ComboBox', idProp); From c68668c2f04e22ddbc6f0666169fc5fbbe5bde07 Mon Sep 17 00:00:00 2001 From: Genevieve Luyt <11131143+genevieveluyt@users.noreply.github.com> Date: Wed, 15 Apr 2020 15:20:56 -0400 Subject: [PATCH 04/10] Use useToggle hook --- .../components/ComboBox/ComboBox.tsx | 42 ++++++++++++------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx index 9dff6fd8f72..2b2d9c7cba2 100644 --- a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx +++ b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx @@ -1,6 +1,7 @@ import React, {useState, useEffect, useCallback} from 'react'; import {useUniqueId} from '../../../../utilities/unique-id'; +import {useToggle} from '../../../../utilities/use-toggle'; import {OptionList, OptionDescriptor} from '../../../OptionList'; import {ActionList} from '../../../ActionList'; import {Popover, PopoverProps} from '../../../Popover'; @@ -65,7 +66,11 @@ export function ComboBox({ const [navigableOptions, setNavigableOptions] = useState< (OptionDescriptor | ActionListItemDescriptor)[] >([]); - const [popoverActive, setPopoverActive] = useState(false); + const { + value: popoverActive, + setTrue: forcePopoverActiveTrue, + setFalse: forcePopoverActiveFalse, + } = useToggle(false); const [popoverWasActive, setPopoverWasActive] = useState(false); const id = useUniqueId('ComboBox', idProp); @@ -85,16 +90,16 @@ export function ComboBox({ ); const handlePopoverClose = useCallback(() => { - setPopoverActive(false); + forcePopoverActiveFalse(); setPopoverWasActive(false); - }, []); + }, [forcePopoverActiveFalse]); const handlePopoverOpen = useCallback(() => { if (!popoverActive && navigableOptions && navigableOptions.length > 0) { - setPopoverActive(true); + forcePopoverActiveTrue(); setPopoverWasActive(true); } - }, [navigableOptions, popoverActive]); + }, [forcePopoverActiveTrue, navigableOptions, popoverActive]); const visuallyUpdateSelectedOption = useCallback( ( @@ -175,11 +180,16 @@ export function ComboBox({ selected && onSelect(selected); if (!allowMultiple) { resetVisuallySelectedOptions(); - setPopoverActive(false); + forcePopoverActiveFalse(); setPopoverWasActive(false); } }, - [allowMultiple, onSelect, resetVisuallySelectedOptions], + [ + allowMultiple, + forcePopoverActiveFalse, + onSelect, + resetVisuallySelectedOptions, + ], ); const handleSelection = useCallback( @@ -232,19 +242,19 @@ export function ComboBox({ ); const handleFocus = useCallback(() => { - setPopoverActive(true); + forcePopoverActiveTrue(); setPopoverWasActive(true); - }, []); + }, [forcePopoverActiveTrue]); const handleBlur = useCallback(() => { - setPopoverActive(false); + forcePopoverActiveFalse(); setPopoverWasActive(false); resetVisuallySelectedOptions(); - }, [resetVisuallySelectedOptions]); + }, [forcePopoverActiveFalse, resetVisuallySelectedOptions]); const handleClick = useCallback(() => { - !popoverActive && setPopoverActive(true); - }, [popoverActive]); + !popoverActive && forcePopoverActiveTrue(); + }, [forcePopoverActiveTrue, popoverActive]); const updateIndexOfSelectedOption = useCallback( (newOptions: (OptionDescriptor | ActionListItemDescriptor)[]) => { @@ -300,18 +310,20 @@ export function ComboBox({ !contentAfter && !emptyState ) { - setPopoverActive(false); + forcePopoverActiveFalse(); } else if ( popoverWasActive && navigableOptions && navigableOptions.length !== 0 ) { - setPopoverActive(true); + forcePopoverActiveTrue(); } }, [ contentAfter, contentBefore, emptyState, + forcePopoverActiveFalse, + forcePopoverActiveTrue, navigableOptions, popoverWasActive, ]); From 9b3ac03b5b1335df864fc0af290cb2c44dd7d6e8 Mon Sep 17 00:00:00 2001 From: Genevieve Luyt <11131143+genevieveluyt@users.noreply.github.com> Date: Wed, 15 Apr 2020 15:34:28 -0400 Subject: [PATCH 05/10] Cleanup syntax --- .../components/ComboBox/ComboBox.tsx | 48 ++++++------------- 1 file changed, 15 insertions(+), 33 deletions(-) diff --git a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx index 2b2d9c7cba2..5e3456841f1 100644 --- a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx +++ b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx @@ -82,9 +82,8 @@ export function ComboBox({ ): ActionListItemDescriptor[] => { if (before) { return navigableOptions.slice(0, actions.length); - } else { - return navigableOptions.slice(-actions.length); } + return navigableOptions.slice(-actions.length); }, [navigableOptions], ); @@ -95,7 +94,7 @@ export function ComboBox({ }, [forcePopoverActiveFalse]); const handlePopoverOpen = useCallback(() => { - if (!popoverActive && navigableOptions && navigableOptions.length > 0) { + if (!popoverActive && navigableOptions.length > 0) { forcePopoverActiveTrue(); setPopoverWasActive(true); } @@ -119,12 +118,9 @@ export function ComboBox({ const resetVisuallySelectedOptions = useCallback(() => { setSelectedOption(undefined); setSelectedIndex(-1); - navigableOptions && - navigableOptions.forEach( - (option: OptionDescriptor | ActionListItemDescriptor) => { - option.active = false; - }, - ); + navigableOptions.forEach((option) => { + option.active = false; + }); }, [navigableOptions]); const selectOptionAtIndex = useCallback( @@ -144,7 +140,7 @@ export function ComboBox({ ); const selectNextOption = useCallback(() => { - if (!navigableOptions || navigableOptions.length === 0) { + if (navigableOptions.length === 0) { return; } @@ -160,7 +156,7 @@ export function ComboBox({ }, [navigableOptions, selectOptionAtIndex, selectedIndex]); const selectPreviousOption = useCallback(() => { - if (!navigableOptions || navigableOptions.length === 0) { + if (navigableOptions.length === 0) { return; } @@ -213,13 +209,11 @@ export function ComboBox({ const handleDownArrow = useCallback(() => { selectNextOption(); - handlePopoverOpen; - }, [handlePopoverOpen, selectNextOption]); + }, [selectNextOption]); const handleUpArrow = useCallback(() => { selectPreviousOption(); - handlePopoverOpen; - }, [handlePopoverOpen, selectPreviousOption]); + }, [selectPreviousOption]); const handleEnter = useCallback( (event: KeyboardEvent) => { @@ -235,10 +229,8 @@ export function ComboBox({ selectedOption.onAction && selectedOption.onAction(); } } - - handlePopoverOpen; }, - [handlePopoverOpen, handleSelection, popoverActive, selectedOption], + [handleSelection, popoverActive, selectedOption], ); const handleFocus = useCallback(() => { @@ -304,18 +296,13 @@ export function ComboBox({ useEffect(() => { if ( - navigableOptions && navigableOptions.length === 0 && !contentBefore && !contentAfter && !emptyState ) { forcePopoverActiveFalse(); - } else if ( - popoverWasActive && - navigableOptions && - navigableOptions.length !== 0 - ) { + } else if (popoverWasActive && navigableOptions.length !== 0) { forcePopoverActiveTrue(); } }, [ @@ -418,15 +405,10 @@ function assignOptionIds( options: (OptionDescriptor | ActionListItemDescriptor)[], id: string, ): OptionDescriptor[] | ActionListItemDescriptor[] { - return options.map( - ( - option: OptionDescriptor | ActionListItemDescriptor, - optionIndex: number, - ) => ({ - ...option, - id: `${id}-${optionIndex}`, - }), - ); + return options.map((option, optionIndex) => ({ + ...option, + id: `${id}-${optionIndex}`, + })); } function isOption( From f2e496b5cf1499890b60434c7f3d1ef8edbcbf8f Mon Sep 17 00:00:00 2001 From: Genevieve Luyt <11131143+genevieveluyt@users.noreply.github.com> Date: Wed, 15 Apr 2020 15:48:17 -0400 Subject: [PATCH 06/10] Remove tracking previous popover state --- .../components/ComboBox/ComboBox.tsx | 33 ------------------- 1 file changed, 33 deletions(-) diff --git a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx index 5e3456841f1..720b202322e 100644 --- a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx +++ b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx @@ -71,7 +71,6 @@ export function ComboBox({ setTrue: forcePopoverActiveTrue, setFalse: forcePopoverActiveFalse, } = useToggle(false); - const [popoverWasActive, setPopoverWasActive] = useState(false); const id = useUniqueId('ComboBox', idProp); @@ -90,16 +89,8 @@ export function ComboBox({ const handlePopoverClose = useCallback(() => { forcePopoverActiveFalse(); - setPopoverWasActive(false); }, [forcePopoverActiveFalse]); - const handlePopoverOpen = useCallback(() => { - if (!popoverActive && navigableOptions.length > 0) { - forcePopoverActiveTrue(); - setPopoverWasActive(true); - } - }, [forcePopoverActiveTrue, navigableOptions, popoverActive]); - const visuallyUpdateSelectedOption = useCallback( ( newOption: OptionDescriptor | ActionListItemDescriptor, @@ -177,7 +168,6 @@ export function ComboBox({ if (!allowMultiple) { resetVisuallySelectedOptions(); forcePopoverActiveFalse(); - setPopoverWasActive(false); } }, [ @@ -235,12 +225,10 @@ export function ComboBox({ const handleFocus = useCallback(() => { forcePopoverActiveTrue(); - setPopoverWasActive(true); }, [forcePopoverActiveTrue]); const handleBlur = useCallback(() => { forcePopoverActiveFalse(); - setPopoverWasActive(false); resetVisuallySelectedOptions(); }, [forcePopoverActiveFalse, resetVisuallySelectedOptions]); @@ -294,27 +282,6 @@ export function ComboBox({ updateIndexOfSelectedOption(navigableOptions); }, [navigableOptions, updateIndexOfSelectedOption]); - useEffect(() => { - if ( - navigableOptions.length === 0 && - !contentBefore && - !contentAfter && - !emptyState - ) { - forcePopoverActiveFalse(); - } else if (popoverWasActive && navigableOptions.length !== 0) { - forcePopoverActiveTrue(); - } - }, [ - contentAfter, - contentBefore, - emptyState, - forcePopoverActiveFalse, - forcePopoverActiveTrue, - navigableOptions, - popoverWasActive, - ]); - let actionsBeforeMarkup: JSX.Element | undefined; if (actionsBefore && actionsBefore.length > 0) { actionsBeforeMarkup = ( From 786235f541660c8b156497261d995450f12cc5f3 Mon Sep 17 00:00:00 2001 From: Genevieve Luyt <11131143+genevieveluyt@users.noreply.github.com> Date: Wed, 15 Apr 2020 16:16:12 -0400 Subject: [PATCH 07/10] Derive selectedOption from selectedIndex --- .../components/ComboBox/ComboBox.tsx | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx index 720b202322e..c6ca595509a 100644 --- a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx +++ b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx @@ -58,9 +58,6 @@ export function ComboBox({ onSelect, onEndReached, }: ComboBoxProps) { - const [selectedOption, setSelectedOption] = useState< - OptionDescriptor | ActionListItemDescriptor | undefined - >(undefined); const [selectedIndex, setSelectedIndex] = useState(-1); const [selectedOptions, setSelectedOptions] = useState(selected); const [navigableOptions, setNavigableOptions] = useState< @@ -107,7 +104,6 @@ export function ComboBox({ ); const resetVisuallySelectedOptions = useCallback(() => { - setSelectedOption(undefined); setSelectedIndex(-1); navigableOptions.forEach((option) => { option.active = false; @@ -116,18 +112,18 @@ export function ComboBox({ const selectOptionAtIndex = useCallback( (newOptionIndex: number) => { - if (!navigableOptions || navigableOptions.length === 0) { + if (navigableOptions.length === 0) { return; } + const oldSelectedOption = navigableOptions[selectedIndex]; const newSelectedOption = navigableOptions[newOptionIndex]; - visuallyUpdateSelectedOption(newSelectedOption, selectedOption); + visuallyUpdateSelectedOption(newSelectedOption, oldSelectedOption); - setSelectedOption(newSelectedOption); setSelectedIndex(newOptionIndex); }, - [navigableOptions, selectedOption, visuallyUpdateSelectedOption], + [navigableOptions, selectedIndex, visuallyUpdateSelectedOption], ); const selectNextOption = useCallback(() => { @@ -211,7 +207,8 @@ export function ComboBox({ return; } - if (popoverActive && selectedOption) { + if (popoverActive && selectedIndex > -1) { + const selectedOption = navigableOptions[selectedIndex]; if (isOption(selectedOption)) { event.preventDefault(); handleSelection(selectedOption.value); @@ -220,7 +217,7 @@ export function ComboBox({ } } }, - [handleSelection, popoverActive, selectedOption], + [handleSelection, navigableOptions, popoverActive, selectedIndex], ); const handleFocus = useCallback(() => { @@ -238,6 +235,7 @@ export function ComboBox({ const updateIndexOfSelectedOption = useCallback( (newOptions: (OptionDescriptor | ActionListItemDescriptor)[]) => { + const selectedOption = navigableOptions[selectedIndex]; if (selectedOption && newOptions.includes(selectedOption)) { selectOptionAtIndex(newOptions.indexOf(selectedOption)); } else if (selectedIndex > newOptions.length - 1) { @@ -247,10 +245,10 @@ export function ComboBox({ } }, [ + navigableOptions, resetVisuallySelectedOptions, selectOptionAtIndex, selectedIndex, - selectedOption, ], ); @@ -318,9 +316,8 @@ export function ComboBox({ options.length === 0 && emptyState &&
{emptyState}
; - const selectedOptionId = selectedOption - ? `${id}-${selectedIndex}` - : undefined; + const selectedOptionId = + selectedIndex > -1 ? `${id}-${selectedIndex}` : undefined; const context = { id, From b3405e01c94b4041bd3b84bff3a8f6cae2d818f8 Mon Sep 17 00:00:00 2001 From: Genevieve Luyt <11131143+genevieveluyt@users.noreply.github.com> Date: Fri, 17 Apr 2020 15:54:14 -0400 Subject: [PATCH 08/10] Some more cleanup --- .../components/ComboBox/ComboBox.tsx | 37 ++++++------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx index c6ca595509a..001927a17be 100644 --- a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx +++ b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx @@ -72,10 +72,7 @@ export function ComboBox({ const id = useUniqueId('ComboBox', idProp); const getActionsWithIds = useCallback( - ( - actions: ActionListItemDescriptor[], - before?: boolean, - ): ActionListItemDescriptor[] => { + (actions: ActionListItemDescriptor[], before?: boolean) => { if (before) { return navigableOptions.slice(0, actions.length); } @@ -84,10 +81,6 @@ export function ComboBox({ [navigableOptions], ); - const handlePopoverClose = useCallback(() => { - forcePopoverActiveFalse(); - }, [forcePopoverActiveFalse]); - const visuallyUpdateSelectedOption = useCallback( ( newOption: OptionDescriptor | ActionListItemDescriptor, @@ -193,14 +186,6 @@ export function ComboBox({ [allowMultiple, selectOptions, selected], ); - const handleDownArrow = useCallback(() => { - selectNextOption(); - }, [selectNextOption]); - - const handleUpArrow = useCallback(() => { - selectPreviousOption(); - }, [selectPreviousOption]); - const handleEnter = useCallback( (event: KeyboardEvent) => { if (event.keyCode !== Key.Enter) { @@ -220,10 +205,6 @@ export function ComboBox({ [handleSelection, navigableOptions, popoverActive, selectedIndex], ); - const handleFocus = useCallback(() => { - forcePopoverActiveTrue(); - }, [forcePopoverActiveTrue]); - const handleBlur = useCallback(() => { forcePopoverActiveFalse(); resetVisuallySelectedOptions(); @@ -333,18 +314,24 @@ export function ComboBox({ aria-owns={id} aria-controls={id} aria-haspopup - onFocus={handleFocus} + onFocus={forcePopoverActiveTrue} onBlur={handleBlur} tabIndex={0} > - - + + - + Date: Mon, 27 Apr 2020 16:59:20 -0400 Subject: [PATCH 09/10] Better spacing Co-Authored-By: Chloe Rice --- src/components/Autocomplete/components/ComboBox/ComboBox.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx index 001927a17be..f901efeda9a 100644 --- a/src/components/Autocomplete/components/ComboBox/ComboBox.tsx +++ b/src/components/Autocomplete/components/ComboBox/ComboBox.tsx @@ -42,6 +42,7 @@ export interface ComboBoxProps { /** Callback when the end of the list is reached */ onEndReached?(): void; } + export function ComboBox({ id: idProp, options, From 74202a051e4a6245d445b5a67e20409e35623b30 Mon Sep 17 00:00:00 2001 From: Genevieve Luyt <11131143+genevieveluyt@users.noreply.github.com> Date: Tue, 28 Apr 2020 09:57:58 -0400 Subject: [PATCH 10/10] Add entry to changelog --- UNRELEASED.md | 1 + 1 file changed, 1 insertion(+) diff --git a/UNRELEASED.md b/UNRELEASED.md index bf3d7fc7276..2151359a006 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -21,5 +21,6 @@ ### Code quality - Set `importsNotUsedAsValues` to `error` in typescript configuration to force us to be explicit when importing types ([#2901](https://github.com/Shopify/polaris-react/pull/2901)) +- Converted `ComboBox` to a functional component ([#2918](https://github.com/Shopify/polaris-react/pull/2918)) ### Deprecations