fix(CheckboxMultiSelect): change value on checkbox label click (#609)

Author: ariser

src/components/Select/CheckboxMultiSelect.stories.tsx

@@ -27,7 +27,8 @@ const CheckboxMultiSelectExample = ({ childrenType, value, ...props }: Props) =>
   }
   return (
     <CheckboxMultiSelect
-      value={value ? value.split(",") : undefined}
+      value={selectedValues}
+      onSelect={value => setSelectedValues(value)}
       selectLabel="Columns"
       {...props}
     >

src/components/Select/CheckboxMultiSelect.test.tsx

@@ -90,7 +90,7 @@ describe("CheckboxCheckboxMultiSelect", () => {
     expect(queryByTestingText(selectTrigger, "Content3")).not.toBeInTheDocument();
   });
 
-  it("shows allows checking and unchecking", async () => {
+  it("allows checking and unchecking", async () => {
     const { getByTestId } = renderSelect({
       selectLabel: "Select columns",
       value: ["content0", "content1"],
@@ -119,6 +119,59 @@ describe("CheckboxCheckboxMultiSelect", () => {
     );
   });
 
+  it("reports onSelect by clicking on the label", () => {
+    const handleSelect = vi.fn();
+    const { queryByText, getByTestId } = renderSelect({
+      selectLabel: "Select columns",
+      value: ["content0", "content1"],
+      onSelect: handleSelect,
+    });
+
+    const selectTrigger = getByTestId("select-trigger");
+    selectTrigger && fireEvent.click(selectTrigger);
+
+    expect(handleSelect).not.toHaveBeenCalled();
+    const content0Label = queryByText("Content0");
+    content0Label && fireEvent.click(content0Label);
+    expect(handleSelect).toHaveBeenCalledTimes(1);
+  });
+
+  it("reports onSelect by clicking on the checkbox", () => {
+    const handleSelect = vi.fn();
+    const { getByTestId } = renderSelect({
+      selectLabel: "Select columns",
+      value: ["content0", "content1"],
+      onSelect: handleSelect,
+    });
+
+    const selectTrigger = getByTestId("select-trigger");
+    selectTrigger && fireEvent.click(selectTrigger);
+
+    expect(handleSelect).not.toHaveBeenCalled();
+    const content0Checkbox = screen
+      .getByTestId("multi-select-checkbox-content0")
+      .querySelector("[role='checkbox']");
+    fireEvent.click(content0Checkbox!);
+    expect(handleSelect).toHaveBeenCalledTimes(1);
+  });
+
+  it("reports onSelect by clicking on the menu item", () => {
+    const handleSelect = vi.fn();
+    const { getByTestId } = renderSelect({
+      selectLabel: "Select columns",
+      value: ["content0", "content1"],
+      onSelect: handleSelect,
+    });
+
+    const selectTrigger = getByTestId("select-trigger");
+    selectTrigger && fireEvent.click(selectTrigger);
+
+    expect(handleSelect).not.toHaveBeenCalled();
+    const content0MenuItem = screen.getByTestId("multi-select-checkbox-content0");
+    content0MenuItem && fireEvent.click(content0MenuItem);
+    expect(handleSelect).toHaveBeenCalledTimes(1);
+  });
+
   it("shows errors", () => {
     const { queryByText } = renderSelect({
       error: "Select Error",

src/components/Select/MultiSelect.stories.tsx

@@ -25,7 +25,8 @@ const MultiSelectExample = ({ childrenType, value, ...props }: Props) => {
   }
   return (
     <MultiSelect
-      value={value ? value.split(",") : undefined}
+      value={selectedValues}
+      onSelect={value => setSelectedValues(value)}
       {...props}
     >
       <MultiSelect.Group heading="Group label">

src/components/Select/common/InternalSelect.tsx

@@ -629,15 +629,33 @@ export const MultiSelectCheckboxItem = forwardRef<
   ) => {
     const { highlighted, updateHighlighted, isHidden, selectedValues, onSelect } =
       useOption();
-    const onSelectValue = (evt: MouseEvent<HTMLElement>) => {
+
+    const handleSelect = (evt: MouseEvent<HTMLElement>) => {
       if (!disabled) {
         onSelect(value, undefined, evt);
-        if (typeof onSelectProp == "function") {
+
+        if (typeof onSelectProp === "function") {
           onSelectProp(value, undefined, evt);
         }
       }
     };
-    const onMouseOver = (e: MouseEvent<HTMLDivElement>) => {
+
+    const handleMenuItemClick = (evt: MouseEvent<HTMLElement>) => {
+      // Clicking checkbox label fires another click event on the checkbox input.
+      // They cancel each other out, so we handle checkbox clicks separately,
+      // and this covers the outside area.
+      if (evt.target !== evt.currentTarget) {
+        return;
+      }
+
+      handleSelect(evt);
+    };
+
+    const handleCheckboxClick = (evt: MouseEvent<HTMLButtonElement>): void => {
+      handleSelect(evt);
+    };
+
+    const handleMenuItemMouseOver = (e: MouseEvent<HTMLDivElement>) => {
       if (onMouseOverProp) {
         onMouseOverProp(e);
       }
@@ -649,19 +667,16 @@ export const MultiSelectCheckboxItem = forwardRef<
     if (isHidden(value)) {
       return null;
     }
-    const isChecked = selectedValues.includes(value);
 
-    const onChange = (): void => {
-      onSelect(value);
-    };
+    const isChecked = selectedValues.includes(value);
 
     return (
       <>
         <GenericMenuItem
           {...props}
           data-value={value}
-          onClick={onSelectValue}
-          onMouseOver={onMouseOver}
+          onClick={handleMenuItemClick}
+          onMouseOver={handleMenuItemMouseOver}
           ref={forwardedRef}
           data-disabled={disabled ? true : undefined}
           data-highlighted={highlighted == value ? "true" : undefined}
@@ -692,7 +707,7 @@ export const MultiSelectCheckboxItem = forwardRef<
                   </div>
                 )
               }
-              onClick={onChange}
+              onClick={handleCheckboxClick}
               variant={variant ?? "default"}
             />
           )}
@@ -706,7 +721,7 @@ export const MultiSelectCheckboxItem = forwardRef<
                 data-testid="multi-select-checkbox"
                 disabled={disabled}
                 label={label ?? children}
-                onClick={onChange}
+                onClick={handleCheckboxClick}
                 variant={variant ?? "default"}
               />
             </IconWrapper>
@@ -717,7 +732,7 @@ export const MultiSelectCheckboxItem = forwardRef<
               data-testid="multi-select-checkbox"
               disabled={disabled}
               label={label ?? children}
-              onClick={onChange}
+              onClick={handleCheckboxClick}
               variant={variant ?? "default"}
             />
           )}