From 35ef286a59ab3f9ab1e8e61a5244cb8903245d46 Mon Sep 17 00:00:00 2001 From: Stephen Haberman Date: Tue, 10 Oct 2023 21:55:48 -0500 Subject: [PATCH] fix: Refactor ComboBoxBase to memo more --- src/inputs/SelectField.test.tsx | 36 +++--- src/inputs/internal/ComboBoxBase.tsx | 177 ++++++++------------------- src/utils/rtl.test.tsx | 176 +++++++++++++++----------- 3 files changed, 181 insertions(+), 208 deletions(-) diff --git a/src/inputs/SelectField.test.tsx b/src/inputs/SelectField.test.tsx index aa23677af..b2bcf8c4c 100644 --- a/src/inputs/SelectField.test.tsx +++ b/src/inputs/SelectField.test.tsx @@ -403,20 +403,28 @@ describe("SelectFieldTest", () => { it("supports boolean values properly", async () => { // Given a select field with boolean and an undefined values const onSelect = jest.fn(); - const r = await render( - o.name} - getOptionValue={(o) => o.id} - />, - ); + type Option = { id: undefined | boolean; name: string }; + function Test() { + const [value, setValue] = useState(true); + return ( + + label="label" + value={value} + onSelect={(value) => { + onSelect(value); + setValue(value); + }} + options={[ + { id: undefined, name: "Undefined" }, + { id: false, name: "False" }, + { id: true, name: "True" }, + ]} + getOptionLabel={(o) => o.name} + getOptionValue={(o) => o.id} + /> + ); + } + const r = await render(); // When selecting the `false` option click(r.label); diff --git a/src/inputs/internal/ComboBoxBase.tsx b/src/inputs/internal/ComboBoxBase.tsx index 0b0b84015..3a3031703 100644 --- a/src/inputs/internal/ComboBoxBase.tsx +++ b/src/inputs/internal/ComboBoxBase.tsx @@ -10,7 +10,6 @@ import { ComboBoxInput } from "src/inputs/internal/ComboBoxInput"; import { ListBox } from "src/inputs/internal/ListBox"; import { keyToValue, Value, valueToKey } from "src/inputs/Value"; import { BeamFocusableProps } from "src/interfaces"; -import { areArraysEqual } from "src/utils"; /** Base props for either `SelectField` or `MultiSelectField`. */ export interface ComboBoxBaseProps extends BeamFocusableProps, PresentationFieldProps { @@ -80,7 +79,7 @@ export function ComboBoxBase(props: ComboBoxBaseProps) onSelect, options: propOptions, multiselect = false, - values = [], + values: propValues, nothingSelectedText = "", contrast, disabledOptions, @@ -96,16 +95,22 @@ export function ComboBoxBase(props: ComboBoxBaseProps) // Memoize the callback functions and handle the `unset` option if provided. const getOptionLabel = useCallback( (o: O) => (unsetLabel && o === unsetOption ? unsetLabel : propOptionLabel(o)), - [propOptionLabel, unsetLabel], + // propOptionLabel is basically always a lambda, so don't dep on it + // eslint-disable-next-line react-hooks/exhaustive-deps + [unsetLabel], ); const getOptionValue = useCallback( (o: O) => (unsetLabel && o === unsetOption ? (undefined as V) : propOptionValue(o)), - [propOptionValue, unsetLabel], + // propOptionValue is basically always a lambda, so don't dep on it + // eslint-disable-next-line react-hooks/exhaustive-deps + [unsetLabel], ); const getOptionMenuLabel = useCallback( (o: O) => propOptionMenuLabel ? propOptionMenuLabel(o, Boolean(unsetLabel) && o === unsetOption) : getOptionLabel(o), - [propOptionMenuLabel, unsetLabel, getOptionLabel], + // propOptionMenuLabel is basically always a lambda, so don't dep on it + // eslint-disable-next-line react-hooks/exhaustive-deps + [unsetLabel, getOptionLabel], ); // Call `initializeOptions` to prepend the `unset` option if the `unsetLabel` was provided. @@ -117,44 +122,33 @@ export function ComboBoxBase(props: ComboBoxBaseProps) Array.isArray(propOptions) ? [propOptions, unsetLabel] : [propOptions.current, propOptions.options, unsetLabel], ); + const values = useMemo(() => propValues ?? [], [propValues]); + + const selectedOptions = useMemo(() => { + return options.filter((o) => values.includes(getOptionValue(o))); + }, [options, values, getOptionValue]); + const { contains } = useFilter({ sensitivity: "base" }); const isDisabled = !!disabled; const isReadOnly = !!readOnly; // Do a one-time initialize of fieldState - const [fieldState, setFieldState] = useState>(() => { - const selectedOptions = options.filter((o) => values.includes(getOptionValue(o))); + const [fieldState, setFieldState] = useState(() => { return { - selectedKeys: selectedOptions?.map((o) => valueToKey(getOptionValue(o))) ?? [], - inputValue: getInputValue( - options.filter((o) => values?.includes(getOptionValue(o))), - getOptionLabel, - multiselect, - nothingSelectedText, - ), - filteredOptions: options, - allOptions: options, - selectedOptions, + inputValue: getInputValue(selectedOptions, getOptionLabel, multiselect, nothingSelectedText), + searchValue: undefined, optionsLoading: false, }; }); + const { searchValue } = fieldState; + const filteredOptions = useMemo(() => { + return !searchValue ? options : options.filter((o) => contains(getOptionLabel(o), searchValue)); + }, [options, searchValue, getOptionLabel, contains]); + /** Resets field's input value and filtered options list for cases where the user exits the field without making changes (on Escape, or onBlur) */ function resetField() { - const inputValue = getInputValue( - fieldState.allOptions.filter((o) => values?.includes(getOptionValue(o))), - getOptionLabel, - multiselect, - nothingSelectedText, - ); - // Conditionally reset the value if the current inputValue doesn't match that of the passed in value, or we filtered the list - if (inputValue !== fieldState.inputValue || fieldState.filteredOptions.length !== fieldState.allOptions.length) { - setFieldState((prevState) => ({ - ...prevState, - inputValue, - filteredOptions: prevState.allOptions, - })); - } + setFieldState((prevState) => ({ ...prevState, searchValue: undefined })); } function onSelectionChange(keys: Selection): void { @@ -172,34 +166,12 @@ export function ComboBoxBase(props: ComboBoxBaseProps) ); if (multiselect && keys.size === 0) { - setFieldState({ - ...fieldState, - inputValue: state.isOpen ? "" : nothingSelectedText, - selectedKeys: [], - selectedOptions: [], - }); selectionChanged && onSelect([], []); return; } const selectedKeys = [...keys.values()]; - const selectedOptions = fieldState.allOptions.filter((o) => selectedKeys.includes(valueToKey(getOptionValue(o)))); - const firstSelectedOption = selectedOptions[0]; - - setFieldState((prevState) => ({ - ...prevState, - // If menu is open then reset inputValue to "". Otherwise set inputValue depending on number of options selected. - inputValue: - multiselect && (state.isOpen || selectedKeys.length > 1) - ? "" - : firstSelectedOption - ? getOptionLabel(firstSelectedOption!) - : "", - selectedKeys, - selectedOptions, - filteredOptions: fieldState.allOptions, - })); - + const selectedOptions = options.filter((o) => selectedKeys.includes(valueToKey(getOptionValue(o)))); selectionChanged && onSelect(selectedKeys.map(keyToValue) as V[], selectedOptions); if (!multiselect) { @@ -210,11 +182,7 @@ export function ComboBoxBase(props: ComboBoxBaseProps) function onInputChange(value: string) { if (value !== fieldState.inputValue) { - setFieldState((prevState) => ({ - ...prevState, - inputValue: value, - filteredOptions: fieldState.allOptions.filter((o) => contains(getOptionLabel(o), value)), - })); + setFieldState((prevState) => ({ ...prevState, inputValue: value, searchValue: value })); } } @@ -232,7 +200,6 @@ export function ComboBoxBase(props: ComboBoxBaseProps) maybeInitLoad(); firstOpen.current = false; } - // When using the multiselect field, always empty the input upon open. if (multiselect && isOpen) { setFieldState((prevState) => ({ ...prevState, inputValue: "" })); @@ -255,7 +222,7 @@ export function ComboBoxBase(props: ComboBoxBaseProps) ...otherProps, disabledKeys: Object.keys(disabledOptionsWithReasons), inputValue: fieldState.inputValue, - items: fieldState.filteredOptions, + items: filteredOptions, isDisabled, isReadOnly, onInputChange, @@ -286,65 +253,34 @@ export function ComboBoxBase(props: ComboBoxBaseProps) }, }); + const selectedKeys = useMemo(() => { + return selectedOptions.map((o) => valueToKey(getOptionValue(o))); + }, [selectedOptions, getOptionValue]); // @ts-ignore - `selectionManager.state` exists, but not according to the types state.selectionManager.state = useMultipleSelectionState({ selectionMode: multiselect ? "multiple" : "single", // Do not allow an empty selection if single select mode disallowEmptySelection: !multiselect, - selectedKeys: fieldState.selectedKeys, + selectedKeys, onSelectionChange, }); - // Ensure we reset if the field's values change and the user is not actively selecting options. - useEffect( - () => { - if (!state.isOpen && !areArraysEqual(values, fieldState.selectedKeys)) { - setFieldState((prevState) => { - const selectedOptions = prevState.allOptions.filter((o) => values?.includes(getOptionValue(o))); - return { - ...prevState, - selectedKeys: selectedOptions?.map((o) => valueToKey(getOptionValue(o))) ?? [], - inputValue: - selectedOptions.length === 1 - ? getOptionLabel(selectedOptions[0]) - : multiselect && selectedOptions.length === 0 - ? nothingSelectedText - : "", - selectedOptions: selectedOptions, - }; - }); - } - }, - // TODO: validate this eslint-disable. It was automatically ignored as part of https://app.shortcut.com/homebound-team/story/40033/enable-react-hooks-exhaustive-deps-for-react-projects - // eslint-disable-next-line react-hooks/exhaustive-deps - [values], - ); - - // Re-sync fieldState.allOptions - useEffect( - () => { - setFieldState((prevState) => { - const selectedOptions = options.filter((o) => values?.includes(getOptionValue(o))); - return { - ...prevState, - selectedKeys: selectedOptions?.map((o) => valueToKey(getOptionValue(o))) ?? [], - inputValue: - selectedOptions.length === 1 - ? getOptionLabel(selectedOptions[0]) - : multiselect && selectedOptions.length === 0 - ? nothingSelectedText - : "", - selectedOptions: selectedOptions, - filteredOptions: options, - allOptions: options, - }; - }); - }, - // We're primarily only re-setting `allOptions`, and so recalc selected as well, but we don't - // want to depend on values/etc., b/c we'll defer to their useEffects to update their state - // eslint-disable-next-line react-hooks/exhaustive-deps - [options], - ); + // Reset inputValue when closed or selected changes + useEffect(() => { + if (state.isOpen && multiselect) { + // While the multiselect is open, let the user keep typing + setFieldState((prevState) => ({ + ...prevState, + inputValue: "", + searchValue: "", + })); + } else { + setFieldState((prevState) => ({ + ...prevState, + inputValue: getInputValue(selectedOptions, getOptionLabel, multiselect, nothingSelectedText), + })); + } + }, [state.isOpen, selectedOptions, getOptionLabel, multiselect, nothingSelectedText]); // For the most part, the returned props contain `aria-*` and `id` attributes for accessibility purposes. const { @@ -396,7 +332,7 @@ export function ComboBoxBase(props: ComboBoxBaseProps) listBoxRef={listBoxRef} state={state} labelProps={labelProps} - selectedOptions={fieldState.selectedOptions} + selectedOptions={selectedOptions} getOptionValue={getOptionValue} getOptionLabel={getOptionLabel} contrast={contrast} @@ -419,7 +355,7 @@ export function ComboBoxBase(props: ComboBoxBaseProps) positionProps={positionProps} state={state} listBoxRef={listBoxRef} - selectedOptions={fieldState.selectedOptions} + selectedOptions={selectedOptions} getOptionLabel={getOptionLabel} getOptionValue={(o) => valueToKey(getOptionValue(o))} contrast={contrast} @@ -433,12 +369,11 @@ export function ComboBoxBase(props: ComboBoxBaseProps) ); } -type FieldState = { - selectedKeys: Key[]; +type FieldState = { inputValue: string; - filteredOptions: O[]; - selectedOptions: O[]; - allOptions: O[]; + // We need separate `searchValue` vs. `inputValue` b/c we might be showing the + // currently-loaded option in the input, without the user having typed a filter yet. + searchValue: string | undefined; optionsLoading: boolean; }; @@ -510,7 +445,3 @@ export function disabledOptionToKeyedTuple( return [valueToKey(disabledOption), undefined]; } } - -function asArray(arrayOrElement: E[] | E | undefined): E[] { - return Array.isArray(arrayOrElement) ? arrayOrElement : arrayOrElement ? [arrayOrElement] : []; -} diff --git a/src/utils/rtl.test.tsx b/src/utils/rtl.test.tsx index d4c450f54..eca686488 100644 --- a/src/utils/rtl.test.tsx +++ b/src/utils/rtl.test.tsx @@ -1,3 +1,4 @@ +import { useState } from "react"; import { MultiSelectField, NestedOption, SelectField, TreeSelectField } from "src/inputs"; import { HasIdAndName } from "src/types"; import { getOptions, getSelected, render, select, selectAndWait } from "src/utils/rtl"; @@ -6,18 +7,25 @@ describe("rtl", () => { it("can use select helpers and select an option via value on SelectField", async () => { const onSelect = jest.fn(); // Given the SelectField - const r = await render( - , - ); + function Test() { + const [value, setValue] = useState(); + return ( + { + setValue(value); + onSelect(value, option); + }} + options={[ + { id: "1", name: "One" }, + { id: "2", name: "Two" }, + { id: "3", name: "Three" }, + ]} + /> + ); + } + const r = await render(); // Then the getOptions helper returns the correct options expect(getOptions(r.number)).toEqual(["One", "Two", "Three"]); @@ -89,18 +97,25 @@ describe("rtl", () => { it("can use select helpers and select an option via value on MultiSelectField", async () => { const onSelect = jest.fn(); // Given the MultiSelectField - const r = await render( - , - ); + function Test() { + const [values, setValues] = useState([]); + return ( + { + setValues(values); + onSelect(values, options); + }} + options={[ + { id: "1", name: "One" }, + { id: "2", name: "Two" }, + { id: "3", name: "Three" }, + ]} + /> + ); + } + const r = await render(); // Then the getOptions helper returns the correct options expect(getOptions(r.number)).toEqual(["One", "Two", "Three"]); @@ -127,18 +142,25 @@ describe("rtl", () => { it("can select options via label on MultiSelectField", async () => { const onSelect = jest.fn(); // Given the MultiSelectField - const r = await render( - , - ); + function Test() { + const [values, setValues] = useState([]); + return ( + { + setValues(values); + onSelect(values, options); + }} + options={[ + { id: "1", name: "One" }, + { id: "2", name: "Two" }, + { id: "3", name: "Three" }, + ]} + /> + ); + } + const r = await render(); // When selecting options by label select(r.number, ["One", "Three"]); @@ -215,30 +237,35 @@ describe("rtl", () => { expect(getSelected(r.number)).toEqual("One One"); }); - // TODO: validate this eslint-disable with https://app.shortcut.com/homebound-team/story/40045 - // eslint-disable-next-line jest/no-identical-title - it("can select options via label on MultiSelectField", async () => { + it("can select options via label on TreeSelectField", async () => { const onSelect = jest.fn(); // Given the TreeSelectField - const r = await render( - onSelect(all)} - options={ - [ - { - id: "1", - name: "One", - children: [ - { id: "1.1", name: "One One" }, - { id: "1.2", name: "One Two" }, - ], - }, - ] as NestedOption[] - } - />, - ); + function Test() { + const [values, setValues] = useState([]); + return ( + { + setValues(all.values); + onSelect(all); + }} + options={ + [ + { + id: "1", + name: "One", + children: [ + { id: "1.1", name: "One One" }, + { id: "1.2", name: "One Two" }, + ], + }, + ] as NestedOption[] + } + /> + ); + } + const r = await render(); // When selecting an option by its label select(r.number, ["One One"]); // Then the onSelect handler is called with the correct values @@ -285,19 +312,26 @@ describe("rtl", () => { it("can use select helpers on multiline SelectField", async () => { const onSelect = jest.fn(); // Given the SelectField is multline - const r = await render( - , - ); + function Test() { + const [value, setValue] = useState(); + return ( + { + setValue(value); + onSelect(value, option); + }} + options={[ + { id: "1", name: "One" }, + { id: "2", name: "Two" }, + { id: "3", name: "Three" }, + ]} + multiline + /> + ); + } + const r = await render(); // Then the getOptions helper returns the correct options expect(getOptions(r.number)).toEqual(["One", "Two", "Three"]); // When selecting an option