Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Refactor ComboBoxBase to memo more #964

Merged
merged 1 commit into from
Oct 12, 2023
Merged

fix: Refactor ComboBoxBase to memo more #964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 22 additions & 14 deletions src/inputs/SelectField.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<SelectField
label="label"
value={true}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this refactoring, this value could stay hard-coded and ComboBoxBase had its own internal version of values that could drift from the value prop.

But now, kinda on accident but I think it makes sense, the internal value is always derived from the value prop, so it behaves more like a controlled component.

Which is fine, afaict not a breaking change, but meant these tests that were hard-coding value=true needed to change.

onSelect={onSelect}
options={[
{ id: undefined, name: "Undefined" },
{ id: false, name: "False" },
{ id: true, name: "True" },
]}
getOptionLabel={(o) => o.name}
getOptionValue={(o) => o.id}
/>,
);
type Option = { id: undefined | boolean; name: string };
function Test() {
const [value, setValue] = useState<boolean | undefined>(true);
return (
<SelectField<Option, boolean | undefined>
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(<Test />);

// When selecting the `false` option
click(r.label);
Expand Down
177 changes: 54 additions & 123 deletions src/inputs/internal/ComboBoxBase.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<O, V extends Value> extends BeamFocusableProps, PresentationFieldProps {
Expand Down Expand Up @@ -80,7 +79,7 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
onSelect,
options: propOptions,
multiselect = false,
values = [],
values: propValues,
nothingSelectedText = "",
contrast,
disabledOptions,
Expand All @@ -96,16 +95,22 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
// 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.
Expand All @@ -117,44 +122,33 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
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<FieldState<O>>(() => {
const selectedOptions = options.filter((o) => values.includes(getOptionValue(o)));
const [fieldState, setFieldState] = useState<FieldState>(() => {
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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filteredOptiosn, allOptions, and selectedOptions, and selectedKeys are now all derived from the useMemo-d options / values or what not.

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 {
Expand All @@ -172,34 +166,12 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
);

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) {
Expand All @@ -210,11 +182,7 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)

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 }));
}
}

Expand All @@ -232,7 +200,6 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
maybeInitLoad();
firstOpen.current = false;
}

// When using the multiselect field, always empty the input upon open.
if (multiselect && isOpen) {
setFieldState((prevState) => ({ ...prevState, inputValue: "" }));
Expand All @@ -255,7 +222,7 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
...otherProps,
disabledKeys: Object.keys(disabledOptionsWithReasons),
inputValue: fieldState.inputValue,
items: fieldState.filteredOptions,
items: filteredOptions,
isDisabled,
isReadOnly,
onInputChange,
Expand Down Expand Up @@ -286,65 +253,34 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
},
});

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda surprised, this useEffect ends up being a pretty cute place to handle resetting inputValue as various conditions change.

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 {
Expand Down Expand Up @@ -396,7 +332,7 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
listBoxRef={listBoxRef}
state={state}
labelProps={labelProps}
selectedOptions={fieldState.selectedOptions}
selectedOptions={selectedOptions}
getOptionValue={getOptionValue}
getOptionLabel={getOptionLabel}
contrast={contrast}
Expand All @@ -419,7 +355,7 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
positionProps={positionProps}
state={state}
listBoxRef={listBoxRef}
selectedOptions={fieldState.selectedOptions}
selectedOptions={selectedOptions}
getOptionLabel={getOptionLabel}
getOptionValue={(o) => valueToKey(getOptionValue(o))}
contrast={contrast}
Expand All @@ -433,12 +369,11 @@ export function ComboBoxBase<O, V extends Value>(props: ComboBoxBaseProps<O, V>)
);
}

type FieldState<O> = {
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;
Comment on lines +374 to +376
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Nice, that does help keep this a bit cleaner

optionsLoading: boolean;
};

Expand Down Expand Up @@ -510,7 +445,3 @@ export function disabledOptionToKeyedTuple(
return [valueToKey(disabledOption), undefined];
}
}

function asArray<E>(arrayOrElement: E[] | E | undefined): E[] {
return Array.isArray(arrayOrElement) ? arrayOrElement : arrayOrElement ? [arrayOrElement] : [];
}
Loading
Loading