Make select all/none search-aware (metabase#48086)

* Fix cancelling of editing of a metric * Fixes * Fixes * Fixes
MeharG811 · Sep 24, 2024 · 957f616 · 957f616
1 parent 1e45098
commit 957f616
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 48 deletions.
diff --git a/frontend/src/metabase/components/ListField/ListField.tsx b/frontend/src/metabase/components/ListField/ListField.tsx
@@ -61,8 +61,6 @@ export const ListField = ({
   const [filter, setFilter] = useState("");
   const waitTime = useContext(waitTimeContext);
   const debouncedFilter = useDebouncedValue(filter, waitTime);
-  const isAll = selectedValues.size === sortedOptions.length;
-  const isNone = selectedValues.size === 0;
 
   const filteredOptions = useMemo(() => {
     const formattedFilter = debouncedFilter.trim().toLowerCase();
@@ -89,6 +87,12 @@ export const ListField = ({
     });
   }, [augmentedOptions, debouncedFilter, sortedOptions]);
 
+  const selectedFilteredOptions = filteredOptions.filter(([value]) =>
+    selectedValues.has(value),
+  );
+  const isAll = selectedFilteredOptions.length === filteredOptions.length;
+  const isNone = selectedFilteredOptions.length === 0;
+
   const shouldShowEmptyState =
     augmentedOptions.length > 0 && filteredOptions.length === 0;
 
@@ -114,14 +118,16 @@ export const ListField = ({
     setFilter(e.target.value);
 
   const handleToggleAll = () => {
-    if (isAll) {
-      setSelectedValues(new Set());
-      onChange([]);
-    } else {
-      const allValues = sortedOptions.map(([value]) => value);
-      setSelectedValues(new Set(allValues));
-      onChange(allValues);
-    }
+    const newSelectedValuesSet = new Set(selectedValues);
+    filteredOptions.forEach(([value]) => {
+      if (isAll) {
+        newSelectedValuesSet.delete(value);
+      } else {
+        newSelectedValuesSet.add(value);
+      }
+    });
+    onChange(Array.from(newSelectedValuesSet));
+    setSelectedValues(newSelectedValuesSet);
   };
 
   return (

diff --git a/frontend/src/metabase/components/ListField/ListField.unit.spec.tsx b/frontend/src/metabase/components/ListField/ListField.unit.spec.tsx
@@ -76,23 +76,24 @@ describe("ListField", () => {
     expect(onChange).toHaveBeenCalledWith(allValues);
   });
 
-  it("should allow to select all options after search", async () => {
+  it("should allow to select only visible options after search", async () => {
     const { onChange } = setup({
-      value: [],
+      value: ["Doohickey", "Gadget"],
       options: allOptions,
     });
 
-    await userEvent.type(
-      screen.getByPlaceholderText("Search the list"),
-      allValues[0],
-    );
-    expect(screen.getByLabelText(allValues[0])).toBeInTheDocument();
-    expect(screen.queryByLabelText(allValues[1])).not.toBeInTheDocument();
+    await userEvent.type(screen.getByPlaceholderText("Search the list"), "get");
+    expect(screen.getByLabelText("Gadget")).toBeInTheDocument();
+    expect(screen.getByLabelText("Widget")).toBeInTheDocument();
+    expect(screen.queryByLabelText("Gizmo")).not.toBeInTheDocument();
+    expect(screen.queryByLabelText("Doohickey")).not.toBeInTheDocument();
 
     const checkbox = screen.getByLabelText("Select all");
     expect(checkbox).not.toBeChecked();
     await userEvent.click(checkbox);
-    expect(onChange).toHaveBeenCalledWith(allValues);
+    expect(onChange).toHaveBeenCalledWith(["Doohickey", "Gadget", "Widget"]);
+    expect(screen.getByLabelText("Gadget")).toBeChecked();
+    expect(screen.getByLabelText("Widget")).toBeChecked();
   });
 
   it("should allow to deselect all options", async () => {
@@ -110,20 +111,21 @@ describe("ListField", () => {
 
   it("should allow to deselect all options after search", async () => {
     const { onChange } = setup({
-      value: allValues,
+      value: ["Doohickey", "Gadget", "Widget"],
       options: allOptions,
     });
 
     await userEvent.type(
       screen.getByPlaceholderText("Search the list"),
-      allValues[0],
+      "Gadget",
     );
-    expect(screen.getByLabelText(allValues[0])).toBeInTheDocument();
-    expect(screen.queryByLabelText(allValues[1])).not.toBeInTheDocument();
+    expect(screen.getByLabelText("Gadget")).toBeInTheDocument();
+    expect(screen.queryByLabelText("Widget")).not.toBeInTheDocument();
 
     const checkbox = screen.getByLabelText("Select none");
     expect(checkbox).toBeChecked();
     await userEvent.click(checkbox);
-    expect(onChange).toHaveBeenCalledWith([]);
+    expect(onChange).toHaveBeenCalledWith(["Doohickey", "Widget"]);
+    expect(screen.getByLabelText("Gadget")).not.toBeChecked();
   });
 });
diff --git a/frontend/src/metabase/components/ListField/types.ts b/frontend/src/metabase/components/ListField/types.ts
@@ -4,7 +4,7 @@ import type { RowValue } from "metabase-types/api";
 export type Option = any[];
 
 export interface ListFieldProps {
-  onChange: (value: string[]) => void;
+  onChange: (value: RowValue[]) => void;
   value: RowValue[];
   options: Option;
   optionRenderer: (option: any) => JSX.Element;

diff --git a/...etabase/querying/filters/components/FilterValuePicker/ListValuePicker/ListValuePicker.tsx b/...etabase/querying/filters/components/FilterValuePicker/ListValuePicker/ListValuePicker.tsx
@@ -51,25 +51,33 @@ function CheckboxListPicker({
 }: ListValuePickerProps) {
   const [searchValue, setSearchValue] = useState("");
   const [elevatedValues] = useState(selectedValues);
-  const options = getEffectiveOptions(
+  const availableOptions = getEffectiveOptions(
     fieldValues,
     selectedValues,
     elevatedValues,
   );
-  const visibleOptions = searchOptions(options, searchValue);
-  const isAll = options.length === selectedValues.length;
-  const isNone = selectedValues.length === 0;
+  const filteredOptions = searchOptions(availableOptions, searchValue);
+  const selectedValuesSet = new Set(selectedValues);
+  const selectedFilteredOptions = filteredOptions.filter(option =>
+    selectedValuesSet.has(option.value),
+  );
+  const isAll = selectedFilteredOptions.length === filteredOptions.length;
+  const isNone = selectedFilteredOptions.length === 0;
 
   const handleInputChange = (event: ChangeEvent<HTMLInputElement>) => {
     setSearchValue(event.currentTarget.value);
   };
 
   const handleToggleAll = () => {
-    if (isAll) {
-      onChange([]);
-    } else {
-      onChange(options.map(option => option.value));
-    }
+    const newSelectedValuesSet = new Set(selectedValues);
+    filteredOptions.forEach(option => {
+      if (isAll) {
+        newSelectedValuesSet.delete(option.value);
+      } else {
+        newSelectedValuesSet.add(option.value);
+      }
+    });
+    onChange(Array.from(newSelectedValuesSet));
   };
 
   return (
@@ -80,7 +88,7 @@ function CheckboxListPicker({
         autoFocus={autoFocus}
         onChange={handleInputChange}
       />
-      {visibleOptions.length > 0 ? (
+      {filteredOptions.length > 0 ? (
         <Stack>
           <Checkbox
             variant="stacked"
@@ -92,7 +100,7 @@ function CheckboxListPicker({
           />
           <Checkbox.Group value={selectedValues} onChange={onChange}>
             <Stack>
-              {visibleOptions.map(option => (
+              {filteredOptions.map(option => (
                 <Checkbox
                   key={option.value}
                   value={option.value}

diff --git a/...erying/filters/components/FilterValuePicker/ListValuePicker/ListValuePicker.unit.spec.tsx b/...erying/filters/components/FilterValuePicker/ListValuePicker/ListValuePicker.unit.spec.tsx
@@ -75,23 +75,25 @@ describe("ListValuePicker", () => {
       expect(onChange).toHaveBeenCalledWith(allValues);
     });
 
-    it("should allow to select all options after search", async () => {
+    it("should allow to select only visible options after search", async () => {
       const { onChange } = setup({
         fieldValues: allOptions,
-        selectedValues: [],
+        selectedValues: ["Doohickey", "Gadget"],
       });
 
       await userEvent.type(
         screen.getByPlaceholderText("Search the list"),
-        allValues[0],
+        "get",
       );
-      expect(screen.getByLabelText(allValues[0])).toBeInTheDocument();
-      expect(screen.queryByLabelText(allValues[1])).not.toBeInTheDocument();
+      expect(screen.getByLabelText("Gadget")).toBeInTheDocument();
+      expect(screen.getByLabelText("Widget")).toBeInTheDocument();
+      expect(screen.queryByLabelText("Gizmo")).not.toBeInTheDocument();
+      expect(screen.queryByLabelText("Doohickey")).not.toBeInTheDocument();
 
       const checkbox = screen.getByLabelText("Select all");
       expect(checkbox).not.toBeChecked();
       await userEvent.click(checkbox);
-      expect(onChange).toHaveBeenCalledWith(allValues);
+      expect(onChange).toHaveBeenCalledWith(["Doohickey", "Gadget", "Widget"]);
     });
 
     it("should allow to deselect all options", async () => {
@@ -107,23 +109,23 @@ describe("ListValuePicker", () => {
       expect(onChange).toHaveBeenCalledWith([]);
     });
 
-    it("should allow to deselect all options after search", async () => {
+    it("should allow to deselect only visible options after search", async () => {
       const { onChange } = setup({
         fieldValues: allOptions,
-        selectedValues: allValues,
+        selectedValues: ["Doohickey", "Gadget", "Widget"],
       });
 
       await userEvent.type(
         screen.getByPlaceholderText("Search the list"),
-        allValues[0],
+        "Gadget",
       );
-      expect(screen.getByLabelText(allValues[0])).toBeInTheDocument();
-      expect(screen.queryByLabelText(allValues[1])).not.toBeInTheDocument();
+      expect(screen.getByLabelText("Gadget")).toBeInTheDocument();
+      expect(screen.queryByLabelText("Widget")).not.toBeInTheDocument();
 
       const checkbox = screen.getByLabelText("Select none");
       expect(checkbox).toBeChecked();
       await userEvent.click(checkbox);
-      expect(onChange).toHaveBeenCalledWith([]);
+      expect(onChange).toHaveBeenCalledWith(["Doohickey", "Widget"]);
     });
   });
 });