Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
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
2 changes: 1 addition & 1 deletion app/client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -387,4 +387,4 @@
"@types/react": "^17.0.2",
"postcss": "8.4.31"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ function Checkbox(props: CheckboxProps) {
isFocusVisible={isFocusVisible}
isIndeterminate={isIndeterminate}
>
{children}
<span>{children}</span>
<input {...inputProps} {...focusProps} ref={ref} />
<span className={CheckboxClassNameSquare} />
</StyledCheckbox>
Expand Down
4 changes: 4 additions & 0 deletions app/client/packages/design-system/ads/src/Select/Select.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ Select allows users to make single or multiple selections from a list of options

<Canvas of={SelectStories.SelectInvalidStory} />

### Grouping with Checkbox in options

<Canvas of={SelectStories.SelectWithCheckboxAndGroup} />

## Best practices

The select component should:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import React, { useState } from "react";
import { Select, Option } from "./Select";
import { Select, Option, OptGroup } from "./Select";
import { Icon } from "../Icon";
import { Checkbox } from "../Checkbox";
import type { SelectProps } from "./Select.types";
import type { StoryObj } from "@storybook/react";
import type { DefaultOptionType } from "rc-select/lib/Select";

export default {
title: "ADS/Components/Select",
Expand Down Expand Up @@ -970,3 +971,75 @@ export function SelectWithCheckbox() {
</Select>
);
}

const groupOptions = [
{
label: "Group 1",
options: [
{
label: "Option 1",
value: "value 11",
},
{
label: "Very long label to force a line break in the option",
value: "Very long label to force a line break in the option",
},
],
},
{
label: "Group 2",
options: Array.from({ length: 1000 }, (_, i) => ({
label: `Option ${i + 1}`,
value: `value ${i + 1}`,
})),
Comment on lines +991 to +994

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider pagination or virtualization for large option lists.

Generating 1000 options at once could impact performance. Consider implementing pagination or using virtualization for better performance.

},
];

export function SelectWithCheckboxAndGroup() {
Comment thread
KelvinOm marked this conversation as resolved.
const [selectedOptions, setSelectedOptions] = useState<DefaultOptionType[]>(
[],
);

return (
<Select
isMultiSelect
onDeselect={(value, unselectedOption) =>
setSelectedOptions(
selectedOptions.filter((opt) => opt.value !== unselectedOption.value),
)
}
onSelect={(value, newSelectedOption) =>
setSelectedOptions([...selectedOptions, newSelectedOption])
}
placeholder="Select options"
showSearch
value={selectedOptions}
virtual
>
{groupOptions.map((group, groupIndex) => (
<OptGroup key={`${group.label}-${groupIndex}`} label={group.label}>
{group.options.map((option, optionIndex) => (
<Option
key={`${option.value}-${groupIndex}-${optionIndex}`}
label={option.label}
title={option.label}
value={option.value}
>
<Checkbox

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it should all be encapsulated in a component to make this component easier to use. Anyway, it's not for this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the only way to do is make APi like

<Select options={...} />

instead of

<Select>
	<Option>Option1</Option>
	<Option>Option1</Option>
</Select>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This can be a solution, yes. We can also use specialized options with checkboxes.

// making it read only as it is interfering with selection of options of rc-select
isReadOnly
isSelected={Boolean(
selectedOptions.find(
(selectedOption) => selectedOption.value == option.value,
),
)}
>
Comment on lines +1028 to +1036

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using controlled checkbox component.

The isReadOnly prop is used as a workaround. Consider implementing a proper controlled checkbox component.

-<Checkbox
-  isReadOnly
-  isSelected={Boolean(
-    selectedOptions.find(
-      (selectedOption) => selectedOption.value == option.value,
-    ),
-  )}
->
+<Checkbox
+  isSelected={Boolean(
+    selectedOptions.find(
+      (selectedOption) => selectedOption.value === option.value,
+    ),
+  )}
+  onChange={(e) => {
+    const { checked } = e.target;
+    if (checked) {
+      setSelectedOptions([...selectedOptions, option]);
+    } else {
+      setSelectedOptions(
+        selectedOptions.filter((opt) => opt.value !== option.value)
+      );
+    }
+  }}
+>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Checkbox
// making it read only as it is interfering with selection of options of rc-select
isReadOnly
isSelected={Boolean(
selectedOptions.find(
(selectedOption) => selectedOption.value == option.value,
),
)}
>
<Checkbox
isSelected={Boolean(
selectedOptions.find(
(selectedOption) => selectedOption.value === option.value,
),
)}
onChange={(e) => {
const { checked } = e.target;
if (checked) {
setSelectedOptions([...selectedOptions, option]);
} else {
setSelectedOptions(
selectedOptions.filter((opt) => opt.value !== option.value)
);
}
}}
>

{option.label}
</Checkbox>
</Option>
))}
</OptGroup>
))}
</Select>
);
}
60 changes: 53 additions & 7 deletions app/client/packages/design-system/ads/src/Select/Select.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from "react";
import React, { useRef, useState } from "react";
import RCSelect, {
Option as RCOption,
OptGroup as RCOptGroup,
Expand All @@ -12,6 +12,7 @@ import { SelectClassName, SelectDropdownClassName } from "./Select.constants";
import { Tag } from "../Tag";
import type { SelectProps } from "./Select.types";
import { Spinner } from "../Spinner";
import { SearchInput } from "../SearchInput";

/*
TODO:
Expand All @@ -29,15 +30,21 @@ function Select(props: SelectProps) {
isLoading = false,
isMultiSelect,
isValid,
maxTagCount = 2,
maxTagCount = isMultiSelect
? props.value?.length > 1
? "responsive"
: 1
: undefined,
maxTagPlaceholder,
maxTagTextLength = 5,
optionLabelProp = "label",
placeholder = "Please select an option",
showSearch = false,
size = "md",
virtual = false,
...rest
} = props;
const searchRef = useRef<HTMLInputElement>(null);
const [searchValue, setSearchValue] = useState("");

const getMaxTagPlaceholder = (omittedValues: any[]) => {
return `+${omittedValues.length}`;
Expand All @@ -51,6 +58,24 @@ function Select(props: SelectProps) {
return <Icon name="arrow-down-s-line" size="md" />;
}

const handleDropdownVisibleChange = (open: boolean) => {
if (open) {
// this is a hack to get the search input to focus when the dropdown is opened
// the reason is, rc-select does not support putting the search input in the dropdown
// and rc-select focus its native searchinput element on dropdown open, but we need to focus the search input
// so we use a timeout to focus the search input after the dropdown is opened
setTimeout(() => {
if (!searchRef.current) return;

searchRef.current?.focus();
}, 200);

return;
}

setSearchValue("");
};
Comment on lines +60 to +76

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Clean up timeout on component unmount.

The timeout used for focusing the search input should be cleaned up to prevent memory leaks.

Apply this diff to add cleanup:

+const timeoutRef = useRef<NodeJS.Timeout>();

 const handleDropdownVisibleChange = (open: boolean) => {
   if (open) {
-    setTimeout(() => {
+    timeoutRef.current = setTimeout(() => {
       if (!searchRef.current) return;
       searchRef.current?.focus();
     }, 200);
     return;
   }
   setSearchValue("");
 };

+useEffect(() => {
+  return () => {
+    if (timeoutRef.current) {
+      clearTimeout(timeoutRef.current);
+    }
+  };
+}, []);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const handleDropdownVisibleChange = (open: boolean) => {
if (open) {
// this is a hack to get the search input to focus when the dropdown is opened
// the reason is, rc-select does not support putting the search input in the dropdown
// and rc-select focus its native searchinput element on dropdown open, but we need to focus the search input
// so we use a timeout to focus the search input after the dropdown is opened
setTimeout(() => {
if (!searchRef.current) return;
searchRef.current?.focus();
}, 200);
return;
}
setSearchValue("");
};
const timeoutRef = useRef<NodeJS.Timeout>();
const handleDropdownVisibleChange = (open: boolean) => {
if (open) {
// this is a hack to get the search input to focus when the dropdown is opened
// the reason is, rc-select does not support putting the search input in the dropdown
// and rc-select focus its native searchinput element on dropdown open, but we need to focus the search input
// so we use a timeout to focus the search input after the dropdown is opened
timeoutRef.current = setTimeout(() => {
if (!searchRef.current) return;
searchRef.current?.focus();
}, 200);
return;
}
setSearchValue("");
};
useEffect(() => {
return () => {
if (timeoutRef.current) {
clearTimeout(timeoutRef.current);
}
};
}, []);


return (
<RCSelect
{...rest}
Expand All @@ -64,20 +89,41 @@ function Select(props: SelectProps) {
SelectDropdownClassName + `--${size}`,
dropdownClassName,
)}
dropdownRender={(menu: any) => {
return (
<div>
{showSearch && (
<SearchInput
onChange={setSearchValue}
placeholder="Type to search..."
ref={searchRef}
size="md"
value={searchValue}
/>
)}
<div>{menu}</div>
</div>
);
}}
inputIcon={<InputIcon />}
maxTagCount={maxTagCount}
maxTagPlaceholder={maxTagPlaceholder || getMaxTagPlaceholder}
maxTagTextLength={maxTagTextLength}
menuItemSelectedIcon=""
mode={isMultiSelect ? "multiple" : undefined}
mode={isMultiSelect ? "tags" : undefined}
onDropdownVisibleChange={handleDropdownVisibleChange}
optionLabelProp={optionLabelProp}
placeholder={placeholder}
searchValue={searchValue}
showArrow
showSearch={showSearch}
tagRender={(props) => {
if (rest.tagRender) {
return rest.tagRender(props);
}

const { closable, label, onClose } = props;

return (
<Tag isClosable={closable} onClose={onClose}>
<Tag isClosable={closable} kind="info" onClose={onClose}>
{label}
</Tag>
);
Expand Down
Loading