Feat: Component MultiSelect

[ayushnirwal]

Description

Adds Multiselect component

Screenshot/Screencast

Checklist

• I have thoroughly tested this code to the best of my abilities.
• I have reviewed the code myself before requesting a review.
• This code is covered by unit tests to verify that it works as intended.
• The QA of this PR is done by a member of the QA team (to be checked by QA).

Partially fixes #92

[Copilot]

Pull request overview

This PR introduces a new MultiSelect component to the frappe-ui-react component library, allowing users to select multiple options from a dropdown list with search functionality.

Changes:

• Adds a new MultiSelect component with search, select/clear all functionality, and customizable rendering
• Provides TypeScript type definitions for the component's props and options
• Includes Storybook stories demonstrating basic usage and customization options

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 18 comments.

File	Description
packages/frappe-ui-react/src/components/multiSelect/types.ts	Defines TypeScript interfaces for MultiSelectOption and MultiSelectProps
packages/frappe-ui-react/src/components/multiSelect/multiSelect.tsx	Implements the main MultiSelect component using Headless UI's Combobox and Popover
packages/frappe-ui-react/src/components/multiSelect/multiSelect.stories.tsx	Provides Storybook stories showcasing default usage and custom slots
packages/frappe-ui-react/src/components/multiSelect/index.ts	Exports the MultiSelect component and types

[Copilot]

The className has a syntax error with an exclamation mark directly after "justify-between" without a space. This should be "justify-between !" or the exclamation mark should be removed entirely. The exclamation mark syntax appears to be an attempt at using Tailwind's important modifier, but it's incorrectly placed.

Suggested change

	"w-full justify-between!",
	value.length === 0 && "text-ink-gray-4!"
	"w-full justify-between",
	value.length === 0 && "text-ink-gray-4"

[Copilot]

The className has a syntax error with an exclamation mark directly after "text-ink-gray-4" without a space. This should be "text-ink-gray-4 !" or the exclamation mark should be removed. The syntax appears incorrect for Tailwind's important modifier.

Suggested change

	"w-full justify-between!",
	value.length === 0 && "text-ink-gray-4!"
	"w-full justify-between",
	value.length === 0 && "text-ink-gray-4"

[Copilot]

The import path for LoadingIndicator is inconsistent with other components in the codebase. Other components (like autoComplete, button) import it as 'import LoadingIndicator from "../loadingIndicator"' (default import), but this file imports it as a named import. This should be changed to use the default import pattern for consistency.

[Copilot]

The MultiSelect component lacks JSDoc comments explaining its purpose, usage, and parameters. Other components in the codebase may have documentation. Consider adding JSDoc comments to describe the component, especially for complex props like compareFn, renderOption, and renderFooter to help developers understand how to use them.

[Copilot]

The compareFn default implementation uses optional chaining on parameters typed as object, which will always be truthy since objects are passed. The @ts-expect-error comment suppresses the legitimate type error without fixing the underlying issue. Consider typing the parameters more specifically or handling the comparison more safely, similar to how it's done in the AutoComplete component.

Suggested change

	export const MultiSelect: React.FC<MultiSelectProps> = ({
	value = [],
	options,
	placeholder = "Select option",
	hideSearch = false,
	loading = false,
	compareFn = (
	a: NoInfer<MultiSelectOption | null> | object,
	b: NoInfer<MultiSelectOption | null> | object
	//@ts-expect-error -- this is fine since we have specified object type in documentation
	) => a?.value === b?.value,
	const defaultMultiSelectCompareFn = (
	a: NoInfer<MultiSelectOption | null> | object,
	b: NoInfer<MultiSelectOption | null> | object
	): boolean => {
	if (
	a !== null &&
	b !== null &&
	typeof a === "object" &&
	typeof b === "object" &&
	"value" in a &&
	"value" in b
	) {
	return (
	// The runtime checks above ensure that `a` and `b` have a `value` property.
	(a as { value: unknown }).value === (b as { value: unknown }).value
	);
	}
	return a === b;
	};
	export const MultiSelect: React.FC<MultiSelectProps> = ({
	value = [],
	options,
	placeholder = "Select option",
	hideSearch = false,
	loading = false,
	compareFn = defaultMultiSelectCompareFn,

[Copilot]

The import path for Popover is inconsistent with the rest of the codebase. Other components (like autoComplete, datePicker) use 'import { Popover } from "../popover"' (named import from the index file), while this file imports the default export directly from the file: 'import Popover from "../popover/popover"'. This should be changed to match the established convention.

[Copilot]

The selectedOptionObjects computation might be inefficient for large option sets as it uses nested array operations (map + find + filter). Consider optimizing this by creating a Map of options keyed by value for O(1) lookups instead of O(n) find operations for each selected value.

Suggested change

	const selectedOptionObjects = useMemo<MultiSelectOption[]>(() => {
	return value
	.map((val) => options.find((opt) => opt.value === val))
	.filter((opt): opt is MultiSelectOption => opt !== undefined);
	}, [value, options]);
	const optionsMap = useMemo(
	() => new Map(options.map((opt) => [opt.value, opt] as const)),
	[options]
	);
	const selectedOptionObjects = useMemo<MultiSelectOption[]>(() => {
	return value
	.map((val) => optionsMap.get(val))
	.filter((opt): opt is MultiSelectOption => opt !== undefined);
	}, [value, optionsMap]);

[Copilot]

The options array includes an 'img' property that is not defined in the MultiSelectOption interface. While this works in the stories because TypeScript allows excess properties in object literals assigned to variables, it creates confusion about what properties options can have. Consider either adding 'img' to the MultiSelectOption interface or using a different approach for the custom rendering example.

[Copilot]

The type assertion 'as (typeof options)[0]' is used to access the 'img' property, but this is a code smell indicating that the type definitions don't match the actual usage. This type casting bypasses type safety and could lead to runtime errors if the option doesn't have an 'img' property. Consider making the option type more flexible or restructuring the example.

Suggested change

	renderOption={(item) => (
	<div className="flex items-center">
	<span className="mr-2">
	<Avatar image={(item as (typeof options)[0]).img} size="sm" />
	</span>
	{item.label}
	</div>
	)}
	renderOption={(item) => {
	const option = options.find((o) => o.value === item.value);
	return (
	<div className="flex items-center">
	<span className="mr-2">
	{option && <Avatar image={option.img} size="sm" />}
	</span>
	{item.label}
	</div>
	);
	}}

[Copilot]

The filtering logic only searches on the 'label' property but not on the 'value' property. The AutoComplete component in this codebase filters on both label and value. Consider whether filtering should also include the value property for consistency and improved searchability.

Suggested change

	return options.filter((opt) =>
	opt.label.toLowerCase().includes(lowerQuery)
	);
	return options.filter((opt) => {
	const labelMatch = opt.label.toLowerCase().includes(lowerQuery);
	const valueString =
	opt.value !== null && opt.value !== undefined
	? String(opt.value).toLowerCase()
	: "";
	const valueMatch = valueString.includes(lowerQuery);
	return labelMatch || valueMatch;
	});