Refactored bound methods, need to add testing to make sure set state doesn't get called

Author: k2snowman69

src/popover.test.tsx

@@ -259,21 +259,34 @@ describe("Popover", () => {
   });
 
   describe("Uncontrolled component methods", () => {
-    it("Opens on request", () => {
+    let spy: jest.SpyInstance<((isOpen: boolean) => void) | undefined>;
+    beforeEach(() => {
+      defaultProps = {
+        ...defaultProps,
+        isOpen: undefined,
+        onOpenChange: () => {}
+      };
+      spy = jest.spyOn(defaultProps, "onOpenChange");
+    });
+
+    it("Opens by method", () => {
       let wrapper = shallow(<Popover {...defaultProps} />);
 
       let popperHover = wrapper.find(PopperHover);
       expect(popperHover).toHaveLength(1);
       expect(popperHover.hasClass("visible")).toBe(false);
+      expect(spy).not.toHaveBeenCalled();
 
       (wrapper.instance() as Popover).open();
 
       popperHover = wrapper.find(PopperHover);
       expect(popperHover).toHaveLength(1);
       expect(popperHover.hasClass("visible")).toBe(true);
+      expect(spy).toHaveBeenCalledTimes(1);
+      expect(spy).toHaveBeenLastCalledWith(true);
     });
 
-    it("Closes on request", () => {
+    it("Opens by hover, closes on request", () => {
       let wrapper = shallow(<Popover {...defaultProps} />);
 
       let popperHover = wrapper.find(PopperHover);
@@ -285,12 +298,17 @@ describe("Popover", () => {
       popperHover = wrapper.find(PopperHover);
       expect(popperHover).toHaveLength(1);
       expect(popperHover.hasClass("visible")).toBe(true);
+      expect(spy).toHaveBeenCalledTimes(1);
+      expect(spy).toHaveBeenLastCalledWith(true);
 
+      spy.mockReset();
       (wrapper.instance() as Popover).close();
 
       popperHover = wrapper.find(PopperHover);
       expect(popperHover).toHaveLength(1);
       expect(popperHover.hasClass("visible")).toBe(false);
+      expect(spy).toHaveBeenCalledTimes(1);
+      expect(spy).toHaveBeenLastCalledWith(false);
     });
   });
 
@@ -322,7 +340,7 @@ describe("Popover", () => {
       popperHover = wrapper.find(PopperHover);
       expect(popperHover).toHaveLength(1);
       expect(popperHover.hasClass("visible")).toBe(true);
-      expect(spy).toHaveBeenLastCalledWith(true);
+      expect(spy).not.toHaveBeenCalled();
     });
 
     it("Closes by prop", () => {
@@ -341,7 +359,7 @@ describe("Popover", () => {
       popperHover = wrapper.find(PopperHover);
       expect(popperHover).toHaveLength(1);
       expect(popperHover.hasClass("visible")).toBe(false);
-      expect(spy).toHaveBeenLastCalledWith(false);
+      expect(spy).not.toHaveBeenCalled();
     });
   });
 });

src/popover.tsx

@@ -16,6 +16,7 @@ export type TargetType = "click" | "hover";
 export type PopperType = "blur" | "click" | "hover" | "none";
 
 export interface PopoverProps {
+  /// Undefined if uncontrolled. If controlled, true for open, false for closed
   isOpen?: boolean;
   popperClassName?: string;
   popperContent?: React.ReactNode;
@@ -26,6 +27,8 @@ export interface PopoverProps {
   targetType: TargetType;
   wrapperElementProps?: any;
   wrapperElementType?: string;
+  /// If uncontrolled - a callback handler to notify when to change the state
+  /// If controlled - a callback handler to notify when isOpen should be changed
   onOpenChange?: (isOpen: boolean) => void;
 }
 
@@ -55,23 +58,19 @@ export class Popover extends React.Component<PopoverProps, PopoverState> {
   public open() {
     if (this.props.isOpen != null) {
       console.warn(
-        "Popover's open() can only be used when popover control's it's open state, not when `open` is passed as a prop"
+        "Popover's open() can only be used when popover control's it's open state, not when `isOpen` is passed as a prop"
       );
     }
-    this.setState({
-      isOpen: true
-    });
+    this.onChangeOpen(true);
   }
 
   public close() {
     if (this.props.isOpen != null) {
       console.warn(
-        "Popover's open() can only be used when popover control's it's open state, not when `open` is passed as a prop"
+        "Popover's open() can only be used when popover control's it's open state, not when `isOpen` is passed as a prop"
       );
     }
-    this.setState({
-      isOpen: false
-    });
+    this.onChangeOpen(false);
   }
 
   componentDidUpdate(prevProps: PopoverProps, prevState: PopoverState) {
@@ -82,10 +81,7 @@ export class Popover extends React.Component<PopoverProps, PopoverState> {
 
     const { onOpenChange } = this.props;
     if (onOpenChange != null) {
-      if (this.props.isOpen !== prevProps.isOpen) {
-        onOpenChange(this.props.isOpen || false);
-      }
-      if (this.state.isOpen !== prevState.isOpen) {
+      if (this.props.isOpen == null && this.state.isOpen !== prevState.isOpen) {
         onOpenChange(this.state.isOpen);
       }
     }
@@ -113,23 +109,14 @@ export class Popover extends React.Component<PopoverProps, PopoverState> {
   }
 
   private renderTarget() {
-    const {
-      popperType,
-      targetClassName,
-      targetContent,
-      targetType
-    } = this.props;
+    const { targetClassName, targetContent, targetType } = this.props;
 
     switch (targetType) {
       case "click":
         return (
           <TargetClick
             className={targetClassName}
-            onClick={() => {
-              this.setState({
-                isOpen: !this.state.isOpen
-              });
-            }}
+            onClick={this.onReverseState}
             ref={ref => {
               this.targetClickRef = ref;
             }}
@@ -141,17 +128,7 @@ export class Popover extends React.Component<PopoverProps, PopoverState> {
         return (
           <TargetHover
             className={targetClassName}
-            onHoverChange={(isHovering: boolean) => {
-              if (
-                popperType === "hover" ||
-                popperType === "none" ||
-                isHovering
-              ) {
-                this.setState({
-                  isOpen: isHovering
-                });
-              }
-            }}
+            onHoverChange={this.onTargetHoverChange}
           >
             {targetContent}
           </TargetHover>
@@ -161,6 +138,14 @@ export class Popover extends React.Component<PopoverProps, PopoverState> {
     }
   }
 
+  private onTargetHoverChange = (isHovering: boolean) => {
+    const { popperType } = this.props;
+
+    if (popperType === "hover" || popperType === "none" || isHovering) {
+      this.onChangeOpen(isHovering);
+    }
+  };
+
   private renderPopper() {
     const { popperContent, popperOptions, popperType } = this.props;
 
@@ -173,86 +158,107 @@ export class Popover extends React.Component<PopoverProps, PopoverState> {
     }
 
     switch (popperType) {
-      case "click":
+      case "blur":
         return (
-          <PopperClick
+          <PopperBlur
             className={className}
             setScheduleUpdate={this.setScheduleUpdate}
             {...popperOptions}
-            onDismiss={() => {
-              this.setState({
-                isOpen: false
-              });
-            }}
+            onDismiss={this.onPopperBlurDismiss}
           >
             {popperContent}
-          </PopperClick>
+          </PopperBlur>
         );
-      case "hover":
+      case "click":
         return (
-          <PopperHover
+          <PopperClick
             className={className}
             setScheduleUpdate={this.setScheduleUpdate}
             {...popperOptions}
-            onHoverChange={(isHovering: boolean) => {
-              this.setState({
-                isOpen: isHovering
-              });
-            }}
+            onDismiss={this.onClose}
           >
             {popperContent}
-          </PopperHover>
+          </PopperClick>
         );
-      case "none":
+      case "hover":
         return (
           <PopperHover
             className={className}
             setScheduleUpdate={this.setScheduleUpdate}
             {...popperOptions}
-            onHoverChange={(isHovering: boolean) => {}}
+            onHoverChange={this.onChangeOpen}
           >
             {popperContent}
           </PopperHover>
         );
-      case "blur":
+      case "none":
         return (
-          <PopperBlur
+          <PopperHover
             className={className}
             setScheduleUpdate={this.setScheduleUpdate}
             {...popperOptions}
-            onDismiss={(event: MouseEvent) => {
-              if (!this.state.isOpen) {
-                return;
-              }
-              if (this.targetClickRef != null) {
-                let target = ReactDOM.findDOMNode(this.targetClickRef);
-                let isTargetOrChild = false;
-                let sourceElement: Element | null = event.srcElement;
-                while (sourceElement != null) {
-                  if (target == sourceElement) {
-                    isTargetOrChild = true;
-                    break;
-                  }
-                  sourceElement = sourceElement.parentElement;
-                }
-
-                if (isTargetOrChild) {
-                  return;
-                }
-              }
-              this.setState({
-                isOpen: false
-              });
-            }}
+            onHoverChange={() => {}}
           >
             {popperContent}
-          </PopperBlur>
+          </PopperHover>
         );
       default:
         throw new Error("Target type must be either click or hover");
     }
   }
 
+  private onPopperBlurDismiss = (event: MouseEvent) => {
+    if (
+      (this.state.isOpen === false && this.props.isOpen == null) ||
+      this.props.isOpen === false
+    ) {
+      return;
+    }
+    if (this.targetClickRef != null) {
+      let target = ReactDOM.findDOMNode(this.targetClickRef);
+      let isTargetOrChild = false;
+      let sourceElement: null | Element = event.srcElement;
+      while (sourceElement != null) {
+        if (target == sourceElement) {
+          isTargetOrChild = true;
+          break;
+        }
+        sourceElement = sourceElement.parentElement;
+      }
+
+      if (isTargetOrChild) {
+        return;
+      }
+    }
+
+    this.onClose();
+  };
+
+  private onClose = () => {
+    this.onChangeOpen(false);
+  };
+
+  private onReverseState = () => {
+    if (this.props.isOpen == null) {
+      this.onChangeOpen(!this.state.isOpen);
+    } else {
+      this.onChangeOpen(!this.props.isOpen);
+    }
+  };
+
+  private onChangeOpen = (newIsOpen: boolean) => {
+    const { isOpen, onOpenChange } = this.props;
+    if (isOpen != null && onOpenChange != null) {
+      if (isOpen != newIsOpen) {
+        onOpenChange(newIsOpen);
+      }
+    } else if (this.state.isOpen != newIsOpen) {
+      this.setState({
+        isOpen: newIsOpen
+      });
+    }
+  };
+
   private setScheduleUpdate = (ref: null | (() => void)) => {
     if (ref != null) {
       this.scheduleUpdate = ref;