[Overlay] better lifecycle props for all overlay components

packages/core/src/components/context-menu/contextMenu.tsx

@@ -12,18 +12,14 @@ import { AbstractPureComponent } from "../../common/abstractPureComponent";
 import * as Classes from "../../common/classes";
 import { Position } from "../../common/position";
 import { safeInvoke } from "../../common/utils";
+import { IOverlayLifecycleProps } from "../overlay/overlay";
 import { Popover, PopperModifiers } from "../popover/popover";
 
 export interface IOffset {
     left: number;
     top: number;
 }
 
-interface IContextMenuProps {
-    /** Callback invoked after the menu has finished transitioning to a closed state. */
-    didClose: () => void;
-}
-
 interface IContextMenuState {
     isOpen: boolean;
     isDarkTheme: boolean;
@@ -38,7 +34,7 @@ const POPPER_MODIFIERS: PopperModifiers = {
 const TRANSITION_DURATION = 100;
 
 /* istanbul ignore next */
-class ContextMenu extends AbstractPureComponent<IContextMenuProps, IContextMenuState> {
+class ContextMenu extends AbstractPureComponent<IOverlayLifecycleProps, IContextMenuState> {
     public state: IContextMenuState = {
         isDarkTheme: false,
         isOpen: false,
@@ -56,6 +52,7 @@ class ContextMenu extends AbstractPureComponent<IContextMenuProps, IContextMenuS
         return (
             <div className={Classes.CONTEXT_MENU_POPOVER_TARGET} style={this.state.offset}>
                 <Popover
+                    {...this.props}
                     backdropProps={{ onContextMenu: this.handleBackdropContextMenu }}
                     content={content}
                     enforceFocus={false}
@@ -66,7 +63,6 @@ class ContextMenu extends AbstractPureComponent<IContextMenuProps, IContextMenuS
                     onInteraction={this.handlePopoverInteraction}
                     position={Position.RIGHT_TOP}
                     popoverClassName={popoverClassName}
-                    popoverDidClose={this.props.didClose}
                     target={<div />}
                     transitionDuration={TRANSITION_DURATION}
                 />
@@ -122,7 +118,7 @@ export function show(menu: JSX.Element, offset: IOffset, onClose?: () => void, i
         contextMenuElement = document.createElement("div");
         contextMenuElement.classList.add(Classes.CONTEXT_MENU);
         document.body.appendChild(contextMenuElement);
-        contextMenu = ReactDOM.render(<ContextMenu didClose={remove} />, contextMenuElement) as ContextMenu;
+        contextMenu = ReactDOM.render(<ContextMenu onClosed={remove} />, contextMenuElement) as ContextMenu;
     }
 
     contextMenu.show(menu, offset, onClose, isDarkTheme);

packages/core/src/components/overlay/overlay.tsx

@@ -15,7 +15,7 @@ import { IProps } from "../../common/props";
 import { safeInvoke } from "../../common/utils";
 import { Portal } from "../portal/portal";
 
-export interface IOverlayableProps {
+export interface IOverlayableProps extends IOverlayLifecycleProps {
     /**
      * Whether the overlay should acquire application focus when it first opens.
      * @default true
@@ -72,11 +72,26 @@ export interface IOverlayableProps {
     /**
      * A callback that is invoked when user interaction causes the overlay to close, such as
      * clicking on the overlay or pressing the `esc` key (if enabled).
+     *
      * Receives the event from the user's interaction, if there was an event (generally either a
      * mouse or key event). Note that, since this component is controlled by the `isOpen` prop, it
      * will not actually close itself until that prop becomes `false`.
      */
-    onClose?(event?: React.SyntheticEvent<HTMLElement>): void;
+    onClose?: (event?: React.SyntheticEvent<HTMLElement>) => void;
+}
+
+export interface IOverlayLifecycleProps {
+    /** Lifecycle method invoked when an Overlay begins to close. (Specifically, when the close transition begins.) */
+    onClosing?: () => void;
+
+    /** Lifecycle method invoked when an Overlay has finished transitioning to the closed state. */
+    onClosed?: () => void;
+
+    /** Lifecycle method invoked when an Overlay begins to open. */
+    onOpening?: () => void;
+
+    /** Lifecycle method invoked when an Overlay has finished transitioning to the open state. */
+    onOpened?: () => void;
 }
 
 export interface IBackdropProps {
@@ -101,16 +116,6 @@ export interface IBackdropProps {
 }
 
 export interface IOverlayProps extends IOverlayableProps, IBackdropProps, IProps {
-    /** Lifecycle callback invoked after the overlay opens and is mounted in the DOM. */
-    didOpen?: () => any;
-
-    /**
-     * Lifecycle callback invoked after a child element finishes exiting the DOM.
-     * This will be invoked for each child of the `Overlay` except for the backdrop element.
-     * The argument is the underlying HTML element that left the DOM.
-     */
-    didClose?: (node: HTMLElement) => any;
-
     /**
      * Toggles the visibility of the overlay and its children.
      * This prop is required because the component is controlled.
@@ -195,7 +200,7 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
             </TransitionGroup>
         );
         if (usePortal) {
-            return <Portal onChildrenMount={this.handleContentMount}>{transitionGroup}</Portal>;
+            return <Portal>{transitionGroup}</Portal>;
         } else {
             return transitionGroup;
         }
@@ -266,9 +271,16 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
             ) : (
                 <span className={Classes.OVERLAY_CONTENT}>{child}</span>
             );
-        const { transitionDuration, transitionName } = this.props;
+        const { onOpening, onOpened, onClosing, onClosed, transitionDuration, transitionName } = this.props;
         return (
-            <CSSTransition classNames={transitionName} onExited={this.props.didClose} timeout={transitionDuration}>
+            <CSSTransition
+                classNames={transitionName}
+                onEntering={onOpening}
+                onEntered={onOpened}
+                onExiting={onClosing}
+                onExited={onClosed}
+                timeout={transitionDuration}
+            >
                 {decoratedChild}
             </CSSTransition>
         );
@@ -339,9 +351,7 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
             document.addEventListener("mousedown", this.handleDocumentClick);
         }
 
-        if (!this.props.usePortal) {
-            safeInvoke(this.props.didOpen);
-        } else if (this.props.hasBackdrop) {
+        if (this.props.hasBackdrop && this.props.usePortal) {
             // add a class to the body to prevent scrolling of content below the overlay
             document.body.classList.add(Classes.OVERLAY_OPEN);
         }
@@ -378,12 +388,6 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
         }
     };
 
-    private handleContentMount = () => {
-        if (this.props.isOpen) {
-            safeInvoke(this.props.didOpen);
-        }
-    };
-
     private handleDocumentFocus = (e: FocusEvent) => {
         if (
             this.props.enforceFocus &&

packages/core/src/components/popover/popover.tsx

@@ -134,31 +134,11 @@ export interface IPopoverProps extends IOverlayableProps, IProps {
      */
     popoverClassName?: string;
 
-    /**
-     * Callback invoked after the popover closes and has been removed from the DOM.
-     */
-    popoverDidClose?: () => void;
-
-    /**
-     * Callback invoked when the popover opens after it is added to the DOM.
-     */
-    popoverDidOpen?: () => void;
-
     /**
      * Ref supplied to the `Classes.POPOVER` element.
      */
     popoverRef?: (ref: HTMLDivElement | null) => void;
 
-    /**
-     * Callback invoked when a popover begins to close.
-     */
-    popoverWillClose?: () => void;
-
-    /**
-     * Callback invoked before the popover opens.
-     */
-    popoverWillOpen?: () => void;
-
     /**
      * Space-delimited string of class names applied to the
      * portal that holds the popover if `usePortal={true}`.
@@ -248,8 +228,6 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>
     /** DOM element that contains the target. */
     public targetElement: HTMLElement;
 
-    // a flag that is set to true while we are waiting for the underlying Portal to complete rendering
-    private isContentMounting = false;
     private cancelOpenTimeout: () => void;
 
     // a flag that lets us detect mouse movement between the target and popover,
@@ -277,7 +255,7 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>
     }
 
     public render() {
-        const { className, disabled, hasBackdrop, targetClassName, targetElementTag } = this.props;
+        const { className, disabled, targetClassName, targetElementTag } = this.props;
         const { isOpen } = this.state;
         const isHoverInteractionKind = this.isHoverInteractionKind();
 
@@ -333,15 +311,17 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>
                     canEscapeKeyClose={this.props.canEscapeKeyClose}
                     canOutsideClickClose={this.props.interactionKind === PopoverInteractionKind.CLICK}
                     className={this.props.portalClassName}
-                    didClose={this.props.popoverDidClose}
-                    didOpen={this.handleContentMount}
                     enforceFocus={this.props.enforceFocus}
-                    hasBackdrop={hasBackdrop}
-                    usePortal={this.props.usePortal}
+                    hasBackdrop={this.props.hasBackdrop}
                     isOpen={isOpen && !isContentEmpty}
                     onClose={this.handleOverlayClose}
+                    onClosed={this.props.onClosed}
+                    onClosing={this.props.onClosing}
+                    onOpened={this.props.onOpened}
+                    onOpening={this.props.onOpening}
                     transitionDuration={this.props.transitionDuration}
                     transitionName={Classes.POPOVER}
+                    usePortal={this.props.usePortal}
                 >
                     {this.renderPopper(children.content)}
                 </Overlay>
@@ -369,15 +349,6 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>
         }
     }
 
-    public componentWillUpdate(_: IPopoverProps, nextState: IPopoverState) {
-        if (!this.state.isOpen && nextState.isOpen) {
-            this.isContentMounting = true;
-            Utils.safeInvoke(this.props.popoverWillOpen);
-        } else if (this.state.isOpen && !nextState.isOpen) {
-            Utils.safeInvoke(this.props.popoverWillClose);
-        }
-    }
-
     public componentDidUpdate() {
         this.updateDarkParent();
     }
@@ -504,13 +475,6 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>
         }
     }
 
-    private handleContentMount = () => {
-        if (this.isContentMounting) {
-            Utils.safeInvoke(this.props.popoverDidOpen);
-            this.isContentMounting = false;
-        }
-    };
-
     private handleTargetFocus = (e: React.FocusEvent<HTMLElement>) => {
         if (this.props.openOnTargetFocus && this.isHoverInteractionKind()) {
             if (e.relatedTarget == null && !this.lostFocusOnSamePage) {

packages/core/test/overlay/overlayTests.tsx

@@ -86,68 +86,6 @@ describe("<Overlay>", () => {
         overlay.unmount();
     });
 
-    it("invokes didOpen when Overlay is opened", () => {
-        const didOpen = spy();
-        mountWrapper(
-            <Overlay didOpen={didOpen} isOpen={false}>
-                {createOverlayContents()}
-            </Overlay>,
-        );
-        assert.isTrue(didOpen.notCalled, "didOpen invoked when overlay closed");
-
-        wrapper.setProps({ isOpen: true });
-        assert.isTrue(didOpen.calledOnce, "didOpen not invoked when overlay open");
-    });
-
-    it("invokes didOpen when inline Overlay is opened", () => {
-        const didOpen = spy();
-        mountWrapper(
-            <Overlay didOpen={didOpen} isOpen={false} usePortal={false}>
-                {createOverlayContents()}
-            </Overlay>,
-        );
-        assert.isTrue(didOpen.notCalled, "didOpen invoked when overlay closed");
-
-        wrapper.setProps({ isOpen: true });
-        assert.isTrue(didOpen.calledOnce, "didOpen not invoked when overlay open");
-    });
-
-    it("invokes didClose when Overlay is closed", done => {
-        const didClose = spy();
-        mountWrapper(
-            <Overlay didClose={didClose} isOpen={true} transitionDuration={1}>
-                {createOverlayContents()}
-            </Overlay>,
-        );
-        assert.isTrue(didClose.notCalled, "didClose invoked when overlay open");
-
-        wrapper.setProps({ isOpen: false });
-        // didClose relies on transition onExited so we go async for a sec
-        setTimeout(() => {
-            wrapper.update();
-            assert.isTrue(didClose.calledOnce, "didClose not invoked when overlay closed");
-            assert.isFalse(wrapper.find("strong").exists(), "no content");
-            done();
-        });
-    });
-
-    it("invokes didClose when inline Overlay is closed", done => {
-        const didClose = spy();
-        mountWrapper(
-            <Overlay didClose={didClose} isOpen={true} usePortal={false} transitionDuration={1}>
-                {createOverlayContents()}
-            </Overlay>,
-        );
-        assert.isTrue(didClose.notCalled, "didClose invoked when overlay open");
-
-        wrapper.setProps({ isOpen: false });
-        // didClose relies on transition onExited so we go async for a sec
-        setTimeout(() => {
-            assert.isTrue(didClose.calledOnce, "didClose not invoked when overlay closed");
-            done();
-        });
-    });
-
     it("renders portal attached to body when not inline after first opened", () => {
         mountWrapper(<Overlay isOpen={false}>{createOverlayContents()}</Overlay>);
         assert.lengthOf(wrapper.find(Portal), 0, "unexpected Portal");
@@ -477,6 +415,43 @@ describe("<Overlay>", () => {
         }
     });
 
+    it("lifecycle methods called as expected", done => {
+        // these lifecycles are passed directly to CSSTransition from react-transition-group
+        // so we do not need to test these extensively. one integration test should do.
+        const onClosed = spy();
+        const onClosing = spy();
+        const onOpened = spy();
+        const onOpening = spy();
+        wrapper = mountWrapper(
+            <Overlay
+                {...{ onClosed, onClosing, onOpened, onOpening }}
+                isOpen={true}
+                usePortal={false}
+                // transition duration shorter than timeout below to ensure it's done
+                transitionDuration={8}
+            >
+                {createOverlayContents()}
+            </Overlay>,
+        );
+        assert.isTrue(onOpening.calledOnce, "onOpening");
+        assert.isFalse(onOpened.calledOnce, "onOpened not called yet");
+
+        setTimeout(() => {
+            // on*ed called after transition completes
+            assert.isTrue(onOpened.calledOnce, "onOpened");
+
+            wrapper.setProps({ isOpen: false });
+            // on*ing called immediately when prop changes
+            assert.isTrue(onClosing.calledOnce, "onClosing");
+            assert.isFalse(onClosed.calledOnce, "onClosed not called yet");
+
+            setTimeout(() => {
+                assert.isTrue(onClosed.calledOnce, "onOpened");
+                done();
+            }, 10);
+        }, 10);
+    });
+
     let index = 0;
     function createOverlayContents() {
         return <strong id={`overlay-${index++}`}>Overlay content!</strong>;