fix: call onChange in event callback instead of useEffect

chanzuckerberg · Oct 4, 2023 · 8509125 · 8509125
1 parent 11430c1
commit 8509125
Showing 1 changed file with 26 additions and 10 deletions.
diff --git a/packages/components/src/core/Dropdown/index.tsx b/packages/components/src/core/Dropdown/index.tsx
@@ -109,10 +109,6 @@ const Dropdown = <Multiple extends boolean | undefined = false>({
     Value<DefaultDropdownMenuOption, true>
   >([]);
 
-  useEffect(() => {
-    onChange(value);
-  }, [onChange, value]);
-
   useEffect(() => {
     if (isControlled) {
       setValue(propValue);
@@ -203,15 +199,19 @@ const Dropdown = <Multiple extends boolean | undefined = false>({
       }
 
       if (multiple) {
-        setValue(pendingValue as Value<DefaultDropdownMenuOption, Multiple>);
+        setValueAndCallOnChange(
+          pendingValue as Value<DefaultDropdownMenuOption, Multiple>
+        );
       }
     }
   }
 
   function handleClick(event: React.MouseEvent<HTMLElement>) {
     if (open) {
       if (multiple) {
-        setValue(pendingValue as Value<DefaultDropdownMenuOption, Multiple>);
+        setValueAndCallOnChange(
+          pendingValue as Value<DefaultDropdownMenuOption, Multiple>
+        );
       }
 
       setOpen(false);
@@ -221,7 +221,9 @@ const Dropdown = <Multiple extends boolean | undefined = false>({
       }
     } else {
       if (multiple) {
-        setPendingValue(value as MUIValue<true>);
+        setValueAndCallOnChange(
+          pendingValue as Value<DefaultDropdownMenuOption, Multiple>
+        );
       }
 
       setAnchorEl(event.currentTarget);
@@ -245,7 +247,9 @@ const Dropdown = <Multiple extends boolean | undefined = false>({
     }
 
     if (multiple) {
-      setValue(pendingValue as Value<DefaultDropdownMenuOption, Multiple>);
+      setValueAndCallOnChange(
+        newValue as Value<DefaultDropdownMenuOption, Multiple>
+      );
     }
 
     if (anchorEl) {
@@ -267,15 +271,20 @@ const Dropdown = <Multiple extends boolean | undefined = false>({
     if (multiple) {
       if (isTriggerChangeOnOptionClick) {
         setPendingValue(newValue as Value<DefaultDropdownMenuOption, true>);
-        return setValue(newValue as Value<DefaultDropdownMenuOption, Multiple>);
+
+        return setValueAndCallOnChange(
+          newValue as Value<DefaultDropdownMenuOption, Multiple>
+        );
       }
 
       return setPendingValue(
         newValue as Value<DefaultDropdownMenuOption, true>
       );
     }
 
-    setValue(newValue as Value<DefaultDropdownMenuOption, Multiple>);
+    setValueAndCallOnChange(
+      newValue as Value<DefaultDropdownMenuOption, Multiple>
+    );
     setOpen(false);
   }
 
@@ -304,6 +313,13 @@ const Dropdown = <Multiple extends boolean | undefined = false>({
       ? ([] as unknown as Value<DefaultDropdownMenuOption, Multiple>)
       : null;
   }
+
+  function setValueAndCallOnChange(
+    newValue: Value<DefaultDropdownMenuOption, Multiple>
+  ) {
+    setValue(newValue);
+    onChange(newValue);
+  }
 };
 
 export default Dropdown;