Skip to content

Commit

Permalink
fix: Fix the unset label getting dropped.
Browse files Browse the repository at this point in the history
  • Loading branch information
@stephenh
stephenh committed Oct 10, 2023
1 parent 7d73098 commit f645146
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 57 deletions.
27 changes: 22 additions & 5 deletions src/inputs/SelectField.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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={() => {}}
Expand Down Expand Up @@ -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(
<TestSelectField
<SelectField
label="Age"
value={undefined as string | undefined}
unsetLabel="None"
options={options}
onSelect={() => {}}
/>,
);
// 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(
<SelectField<HasIdAndName, string>
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`
Expand Down
114 changes: 62 additions & 52 deletions src/inputs/internal/ComboBoxBase.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
disabled,
readOnly,
onSelect,
options,
options: propOptions,
multiselect = false,
values = [],
nothingSelectedText = "",
Expand All @@ -93,8 +93,6 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
} = 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)),
Expand All @@ -110,24 +108,33 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
[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<FieldState<O>>(() => {
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,
};
});
Expand Down Expand Up @@ -212,9 +219,9 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
}

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 }));
}
}
Expand Down Expand Up @@ -313,43 +320,30 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
[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.
Expand Down Expand Up @@ -475,18 +469,34 @@ function getInputValue<O>(
: "";
}

export function initializeOptions<O>(options: OptionsOrLoad<O>, unsetLabel: string | undefined): OptionsOrLoad<O> {
if (!unsetLabel) {
return options;
}
if (Array.isArray(options)) {
return getOptionsWithUnset(unsetLabel, options);
export function initializeOptions<O, V extends Value>(
optionsOrLoad: OptionsOrLoad<O>,
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<O>(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. */
Expand Down

0 comments on commit f645146

Please sign in to comment.