Ensure Dropdown callback props are merged (jpmorganchase#544)

Co-authored-by: Josh Wooding <[email protected]>

.changeset/nice-crabs-care.md

@@ -0,0 +1,6 @@
+---
+"@jpmorganchase/uitk-lab": minor
+---
+
+Improve types for forwardCallbackProps
+Ensure Dropdown callback props are merged

packages/lab/src/__tests__/__e2e__/dropdown/Dropdown.keyboardNavigation.cy.tsx

@@ -1,4 +1,8 @@
-import { Dropdown, SelectionStrategy } from "@jpmorganchase/uitk-lab";
+import {
+  Dropdown,
+  DropdownButton,
+  SelectionStrategy,
+} from "@jpmorganchase/uitk-lab";
 
 /**
  * Changes applied
@@ -497,5 +501,30 @@ const testSource = ["Bar", "Foo", "Foo Bar", "Baz"];
         cy.get("#test-popup").should("not.exist");
       });
     });
+
+    describe("Given a Dropdown with custom trigger component", () => {
+      it("Should respect callback props on cloned custom trigger", () => {
+        const keyDownSpy = cy.stub().as("keyDownSpy");
+        cy.mount(
+          <Dropdown
+            id="test"
+            placement="bottom-end"
+            source={testSource}
+            triggerComponent={
+              <DropdownButton
+                id="custom-button"
+                tabIndex={0}
+                label="blah"
+                onKeyDown={keyDownSpy}
+              />
+            }
+          ></Dropdown>
+        );
+        cy.get("#custom-button").focus();
+        cy.realPress("ArrowDown");
+        cy.get("@keyDownSpy").should("have.callCount", 1);
+        cy.get("#test-popup").should("exist");
+      });
+    });
   });
 });

packages/lab/src/dropdown/Dropdown.tsx

@@ -19,10 +19,11 @@ import {
 } from "../common-hooks";
 import { List } from "../list/List";
 import { ListProps } from "../list/listTypes";
-import { DropdownBase } from "./DropdownBase";
+import { DropdownBase, MaybeChildProps } from "./DropdownBase";
 import { DropdownButton } from "./DropdownButton";
 import { DropdownBaseProps } from "./dropdownTypes";
 import { useDropdown } from "./useDropdown";
+import { forwardCallbackProps } from "../utils";
 
 export interface DropdownProps<
   Item = "string",
@@ -119,8 +120,6 @@ export const Dropdown = forwardRef(function Dropdown<
         return itemOrItems.map((i) => i.value) as returnType;
       } else if (itemOrItems) {
         return itemOrItems.value as returnType;
-      } else {
-        return null as returnType;
       }
     },
     []
@@ -134,10 +133,14 @@ export const Dropdown = forwardRef(function Dropdown<
       "aria-label": ariaLabel,
     };
     if (triggerComponent) {
-      return cloneElement(triggerComponent, {
-        ...listControlProps,
-        ...ariaProps,
-      });
+      const ownProps = triggerComponent.props as MaybeChildProps;
+      return cloneElement(
+        triggerComponent,
+        forwardCallbackProps(ownProps, {
+          ...listControlProps,
+          ...ariaProps,
+        })
+      );
     } else {
       return (
         <DropdownButton

packages/lab/src/dropdown/DropdownBase.tsx

@@ -24,7 +24,7 @@ import "./Dropdown.css";
 // Any component may be passed as our trigger or popup component.
 // Define the common props that we will act on, if present,
 // so we can type them.
-type MaybeProps = {
+export type MaybeChildProps = {
   className?: string;
   id?: string;
   role?: string;
@@ -126,7 +126,7 @@ export const DropdownBase = forwardRef<HTMLDivElement, DropdownBaseProps>(
         id = defaultId,
         role = defaultRole,
         ...ownProps
-      } = trigger.props as MaybeProps;
+      } = trigger.props as MaybeChildProps;
 
       return cloneElement(
         trigger,
@@ -145,7 +145,7 @@ export const DropdownBase = forwardRef<HTMLDivElement, DropdownBaseProps>(
         id = defaultId,
         width: ownWidth,
         ...ownProps
-      } = popupComponent.props as MaybeProps;
+      } = popupComponent.props as MaybeChildProps;
       return cloneElement(popupComponent, {
         ...ownProps,
         ...restComponentProps,

packages/lab/src/list/List.tsx

@@ -19,7 +19,6 @@ import {
   useCollectionItems,
   useImperativeScrollingAPI,
 } from "../common-hooks";
-import { forwardCallbackProps } from "../utils";
 
 import { ListItem as DefaultListItem, ListItemProxy } from "./ListItem";
 import { ListItemProps, ListProps } from "./listTypes";
@@ -223,7 +222,7 @@ export const List = forwardRef(function List<
     };
     list.push(
       isChildItem ? (
-        cloneElement(value, forwardCallbackProps(value.props, listItemProps))
+        cloneElement(value, listItemProps)
       ) : (
         <ListItem {...listItemProps} />
       )

packages/lab/src/utils/forwardCallbackProps.ts

@@ -1,31 +1,33 @@
-/* 
+/*
   When we clone a React element and inject props, if any of these are
-  callback props, make sure original callback props are also invoked. 
-  
+  callback props, make sure original callback props are also invoked.
+
  React.cloneElement(
     element,
     forwardCallbackProps(element.props, overrideProps)
   )
  */
-export const forwardCallbackProps = (elementProps: any, overrideProps: any) => {
-  const callbackProps = Object.entries<Function>(elementProps).filter(
-    ([, value]) => typeof value === "function"
+
+type Props = Record<string, any>;
+
+export const forwardCallbackProps = <P1 extends Props, P2 extends Props>(
+  ownProps: P1,
+  overrideProps: P2
+): P1 & P2 => {
+  const props = Object.keys(ownProps).reduce<Props>(
+    (map, name) => {
+      const ownProp = ownProps[name];
+      const overrideProp = overrideProps[name];
+      if (typeof ownProp === "function" && typeof overrideProp === "function") {
+        map[name] = (...args: unknown[]) => {
+          ownProp(...args);
+          overrideProp(...args);
+        };
+      }
+      return map;
+    },
+    { ...overrideProps }
   );
 
-  if (callbackProps.some(([name]) => overrideProps[name] !== undefined)) {
-    return {
-      ...overrideProps,
-      ...callbackProps.reduce((map, [name, fn]) => {
-        if (overrideProps[name] && typeof overrideProps[name] === "function") {
-          map[name] = (...args: any) => {
-            overrideProps[name]?.(...args);
-            fn(...args);
-          };
-        }
-        return map;
-      }, {} as any),
-    };
-  } else {
-    return overrideProps;
-  }
+  return props as P1 & P2;
 };