From f645146e0b04091e62dffdaeed9709cc311fdaef Mon Sep 17 00:00:00 2001 From: Stephen Haberman Date: Tue, 10 Oct 2023 18:39:07 -0500 Subject: [PATCH] fix: Fix the unset label getting dropped. --- src/inputs/SelectField.test.tsx | 27 +++++-- src/inputs/internal/ComboBoxBase.tsx | 114 +++++++++++++++------------ 2 files changed, 84 insertions(+), 57 deletions(-) diff --git a/src/inputs/SelectField.test.tsx b/src/inputs/SelectField.test.tsx index fdec47412..aa23677af 100644 --- a/src/inputs/SelectField.test.tsx +++ b/src/inputs/SelectField.test.tsx @@ -189,7 +189,10 @@ describe("SelectFieldTest", () => { value="1" options={{ current: options[0], - load: async () => setLoaded(options), + load: async () => { + await sleep(0); + setLoaded(options); + }, options: loaded, }} onSelect={() => {}} @@ -344,13 +347,27 @@ describe("SelectFieldTest", () => { it("can initially be set to the 'unsetLabel' option", async () => { // Given a Select Field with the value set to `undefined` const r = await render( - {}} + />, + ); + // The input value will be set to the `unsetLabel` + expect(r.age).toHaveValue("None"); + }); + + it("can initially be set to the 'unsetLabel' option when lazy loading options", async () => { + // Given a Select Field with the value set to `undefined` + const r = await render( + label="Age" value={undefined} unsetLabel="None" - options={labelValueOptions} - getOptionLabel={(o) => o.label} - getOptionValue={(o) => o.value} + options={{ current: undefined, load: async () => {}, options: undefined }} + onSelect={() => {}} />, ); // The input value will be set to the `unsetLabel` diff --git a/src/inputs/internal/ComboBoxBase.tsx b/src/inputs/internal/ComboBoxBase.tsx index b64d9d4ee..983713350 100644 --- a/src/inputs/internal/ComboBoxBase.tsx +++ b/src/inputs/internal/ComboBoxBase.tsx @@ -78,7 +78,7 @@ export function ComboBoxBase(props: ComboBoxBaseProps) disabled, readOnly, onSelect, - options, + options: propOptions, multiselect = false, values = [], nothingSelectedText = "", @@ -93,8 +93,6 @@ export function ComboBoxBase(props: ComboBoxBaseProps) } = props; const labelStyle = otherProps.labelStyle ?? fieldProps?.labelStyle ?? "above"; - // Call `initializeOptions` to prepend the `unset` option if the `unsetLabel` was provided. - const maybeOptions = useMemo(() => initializeOptions(options, unsetLabel), [options, unsetLabel]); // Memoize the callback functions and handle the `unset` option if provided. const getOptionLabel = useCallback( (o: O) => (unsetLabel && o === unsetOption ? unsetLabel : propOptionLabel(o)), @@ -110,24 +108,33 @@ export function ComboBoxBase(props: ComboBoxBaseProps) [propOptionMenuLabel, unsetLabel, getOptionLabel], ); + // Call `initializeOptions` to prepend the `unset` option if the `unsetLabel` was provided. + const options = useMemo( + () => initializeOptions(propOptions, getOptionValue, unsetLabel), + // If the caller is using { current, load, options }, memoize on only `current` and `options` values. + // ...and don't bother on memoizing on getOptionValue b/c it's basically always a lambda + // eslint-disable-next-line react-hooks/exhaustive-deps + Array.isArray(propOptions) ? [propOptions, unsetLabel] : [propOptions.current, propOptions.options, unsetLabel], + ); + const { contains } = useFilter({ sensitivity: "base" }); const isDisabled = !!disabled; const isReadOnly = !!readOnly; + // Do a one-time initialize of fieldState const [fieldState, setFieldState] = useState>(() => { - const initOptions = Array.isArray(maybeOptions) ? maybeOptions : asArray(maybeOptions.current); - const selectedOptions = initOptions.filter((o) => values.includes(getOptionValue(o))); + const selectedOptions = options.filter((o) => values.includes(getOptionValue(o))); return { selectedKeys: selectedOptions?.map((o) => valueToKey(getOptionValue(o))) ?? [], inputValue: getInputValue( - initOptions.filter((o) => values?.includes(getOptionValue(o))), + options.filter((o) => values?.includes(getOptionValue(o))), getOptionLabel, multiselect, nothingSelectedText, ), - filteredOptions: initOptions, - allOptions: initOptions, - selectedOptions: selectedOptions, + filteredOptions: options, + allOptions: options, + selectedOptions, optionsLoading: false, }; }); @@ -212,9 +219,9 @@ export function ComboBoxBase(props: ComboBoxBaseProps) } async function maybeInitLoad() { - if (!Array.isArray(maybeOptions)) { + if (!Array.isArray(propOptions)) { setFieldState((prevState) => ({ ...prevState, optionsLoading: true })); - await maybeOptions.load(); + await propOptions.load(); setFieldState((prevState) => ({ ...prevState, optionsLoading: false })); } } @@ -313,43 +320,30 @@ export function ComboBoxBase(props: ComboBoxBaseProps) [values], ); - // When options are an array, then use them as-is. - // If options are an object, then use the `initial` array if the menu has not been opened - // Otherwise, use the current fieldState array options. - const maybeUpdatedOptions = Array.isArray(maybeOptions) - ? maybeOptions - : firstOpen.current === false && !fieldState.optionsLoading - ? maybeOptions.options - : maybeOptions.current; - + // Re-sync fieldState.allOptions useEffect( () => { - // We leave `maybeOptions.initial` as a non-array so that it's stable, but now that we're inside the - // useEffect, array-ize it if needed. - const maybeUpdatedArray = asArray(maybeUpdatedOptions); - if (maybeUpdatedArray !== fieldState.allOptions) { - setFieldState((prevState) => { - const selectedOptions = maybeUpdatedArray.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: maybeUpdatedArray, - allOptions: maybeUpdatedArray, - }; - }); - } + 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, + }; + }); }, - // I started working on fixing this deps array, but seems like `getOptionLabel` & friends - // would very rarely be stable anyway, so going to hold off on further fixes for now... + // 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 - [maybeUpdatedOptions, getOptionLabel, getOptionValue], + [options], ); // For the most part, the returned props contain `aria-*` and `id` attributes for accessibility purposes. @@ -475,18 +469,34 @@ function getInputValue( : ""; } -export function initializeOptions(options: OptionsOrLoad, unsetLabel: string | undefined): OptionsOrLoad { - if (!unsetLabel) { - return options; - } - if (Array.isArray(options)) { - return getOptionsWithUnset(unsetLabel, options); +export function initializeOptions( + optionsOrLoad: OptionsOrLoad, + getOptionValue: (opt: O) => V, + unsetLabel: string | undefined, +): O[] { + if (Array.isArray(optionsOrLoad)) { + const options = optionsOrLoad; + if (!unsetLabel) { + return options; + } else { + return getOptionsWithUnset(unsetLabel, options); + } + } else { + const { options, current } = optionsOrLoad; + const opts: O[] = []; + if (unsetLabel) opts.push(unsetOption as unknown as O); + if (options) opts.push(...options); + // Even if the SelectField has lazy-loaded options, make sure the current value is really in there + if (current) { + const found = options && options.find((o) => getOptionValue(o) === getOptionValue(current)); + if (!found) opts.push(current); + } + return opts; } - return { ...options, options: getOptionsWithUnset(unsetLabel, options.options) }; } function getOptionsWithUnset(unsetLabel: string, options: O[] | undefined): O[] { - return [unsetOption as unknown as O, ...(options ? options : [])]; + return [unsetOption as unknown as O, ...asArray(options)]; } /** A marker option to automatically add an "Unset" option to the start of options. */