[Portal] SSR support, one fewer element in Overlay (#2205)

* refactor Portal to support SSR. - created DOM element receives className - remove containerRef prop and associated element * refactor Overlay to not use deleted containerRef prop - use TransitionGroup as the wrapper element!! * conditionally add classes in portal * enable Portal isotest * refactor TransitionGroup usage * ternary style refactors
palantir · Mar 8, 2018 · 2d7f756 · 2d7f756 · blueprint-bot · Mar 8, 2018
1 parent 7ef988a
commit 2d7f756
Show file tree

Hide file tree

Showing 4 changed files with 102 additions and 85 deletions.
diff --git a/packages/core/src/components/overlay/overlay.tsx b/packages/core/src/components/overlay/overlay.tsx
@@ -8,6 +8,7 @@ import * as classNames from "classnames";
 import * as React from "react";
 import { CSSTransition, TransitionGroup } from "react-transition-group";
 
+import { findDOMNode } from "react-dom";
 import * as Classes from "../../common/classes";
 import * as Keys from "../../common/keys";
 import { IProps } from "../../common/props";
@@ -144,7 +145,7 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
     // an HTMLElement that contains the backdrop and any children, to query for focus target
     public containerElement: HTMLElement;
     private refHandlers = {
-        container: (ref: HTMLDivElement) => (this.containerElement = ref),
+        container: (ref: React.ReactInstance) => (this.containerElement = findDOMNode(ref) as HTMLElement),
     };
 
     public constructor(props?: IOverlayProps, context?: any) {
@@ -158,34 +159,15 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
             return null;
         }
 
-        const { children, className, usePortal, isOpen, transitionDuration, transitionName } = this.props;
+        const { children, className, usePortal, isOpen } = this.props;
 
-        const childrenWithTransitions = React.Children.map(children, (child?: React.ReactChild) => {
-            if (child == null || typeof child !== "object") {
-                return child;
-            }
-            // add a special class to each child element that will automatically set the appropriate
-            // CSS position mode under the hood. also, make the container focusable so we can
-            // trap focus inside it (via `enforceFocus`).
-            const decoratedChild = React.cloneElement(child, {
-                className: classNames(child.props.className, Classes.OVERLAY_CONTENT),
-                tabIndex: 0,
-            });
-            return (
-                <CSSTransition classNames={transitionName} timeout={transitionDuration}>
-                    {decoratedChild}
-                </CSSTransition>
-            );
-        });
+        // TransitionGroup types require single array of children; does not support nested arrays.
+        // So we must collapse backdrop and children into one array, and every item must be wrapped in a
+        // Transition element (no ReactText allowed).
+        const childrenWithTransitions = isOpen ? React.Children.map(children, this.maybeRenderChild) : [];
+        childrenWithTransitions.unshift(this.maybeRenderBackdrop());
 
-        const transitionGroup = (
-            <TransitionGroup appear={true}>
-                {this.maybeRenderBackdrop()}
-                {isOpen ? childrenWithTransitions : null}
-            </TransitionGroup>
-        );
-
-        const mergedClassName = classNames(
+        const containerClasses = classNames(
             Classes.OVERLAY,
             {
                 [Classes.OVERLAY_OPEN]: isOpen,
@@ -194,27 +176,21 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
             className,
         );
 
-        const elementProps = {
-            className: mergedClassName,
-            onKeyDown: this.handleKeyDown,
-        };
-
+        const transitionGroup = (
+            <TransitionGroup
+                appear={true}
+                className={containerClasses}
+                component="div"
+                onKeyDown={this.handleKeyDown}
+                ref={this.refHandlers.container}
+            >
+                {childrenWithTransitions}
+            </TransitionGroup>
+        );
         if (usePortal) {
-            return (
-                <Portal
-                    {...elementProps}
-                    containerRef={this.refHandlers.container}
-                    onChildrenMount={this.handleContentMount}
-                >
-                    {transitionGroup}
-                </Portal>
-            );
+            return <Portal onChildrenMount={this.handleContentMount}>{transitionGroup}</Portal>;
         } else {
-            return (
-                <span {...elementProps} ref={this.refHandlers.container}>
-                    {transitionGroup}
-                </span>
-            );
+            return transitionGroup;
         }
     }
 
@@ -267,6 +243,30 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
         });
     }
 
+    private maybeRenderChild = (child?: React.ReactChild) => {
+        if (child == null) {
+            return null;
+        }
+        // add a special class to each child element that will automatically set the appropriate
+        // CSS position mode under the hood. also, make the container focusable so we can
+        // trap focus inside it (via `enforceFocus`).
+        const decoratedChild =
+            typeof child === "object" ? (
+                React.cloneElement(child, {
+                    className: classNames(child.props.className, Classes.OVERLAY_CONTENT),
+                    tabIndex: 0,
+                })
+            ) : (
+                <span className={Classes.OVERLAY_CONTENT}>{child}</span>
+            );
+        const { transitionDuration, transitionName } = this.props;
+        return (
+            <CSSTransition classNames={transitionName} timeout={transitionDuration}>
+                {decoratedChild}
+            </CSSTransition>
+        );
+    };
+
     private maybeRenderBackdrop() {
         const {
             backdropClassName,
@@ -289,7 +289,7 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
                 </CSSTransition>
             );
         } else {
-            return undefined;
+            return null;
         }
     }
 

diff --git a/packages/core/src/components/portal/portal.tsx b/packages/core/src/components/portal/portal.tsx
@@ -9,17 +9,10 @@ import * as ReactDOM from "react-dom";
 
 import * as Classes from "../../common/classes";
 import * as Errors from "../../common/errors";
-import { IProps, removeNonHTMLProps } from "../../common/props";
+import { IProps } from "../../common/props";
 import { safeInvoke } from "../../common/utils";
 
 export interface IPortalProps extends IProps, React.HTMLProps<HTMLDivElement> {
-    /**
-     * A React `ref` handler callback for the detached container root element.
-     * As this component renders its contents into a separate container, the result of the `ref`
-     * prop is not backed by a DOM node. Hence this callback is necessary to get the real DOM node.
-     */
-    containerRef?: (ref: HTMLDivElement) => void;
-
     /**
      * Callback invoked when the children of this `Portal` have been added to the DOM.
      */
@@ -31,7 +24,7 @@ export interface IPortalState {
 }
 
 export interface IPortalContext {
-    /** Additional class to add to portal element */
+    /** Additional CSS classes to add to all `Portal` elements in this React context. */
     blueprintPortalClassName?: string;
 }
 
@@ -56,36 +49,53 @@ export class Portal extends React.Component<IPortalProps, IPortalState> {
     public context: IPortalContext;
     public state: IPortalState = { hasMounted: false };
 
-    private targetElement: HTMLElement;
-
-    constructor(props: IPortalProps, context: IPortalContext) {
-        super(props, context);
-        this.targetElement = document.createElement("div");
-        this.targetElement.classList.add(Classes.PORTAL);
-        if (context.blueprintPortalClassName != null) {
-            this.targetElement.classList.add(context.blueprintPortalClassName);
-        }
-    }
+    private portalElement: HTMLElement;
 
     public render() {
-        // Only render `children` once this component has mounted, so they are immediately attached to the DOM tree and
-        // can do DOM things like measuring or `autoFocus`. See long comment on componentDidMount in
-        // https://reactjs.org/docs/portals.html#event-bubbling-through-portals
-        return ReactDOM.createPortal(
-            <div {...removeNonHTMLProps(this.props)} ref={this.props.containerRef}>
-                {this.state.hasMounted ? this.props.children : null}
-            </div>,
-            this.targetElement,
-        );
+        // Only render `children` once this component has mounted in a browser environment, so they are
+        // immediately attached to the DOM tree and can do DOM things like measuring or `autoFocus`.
+        // See long comment on componentDidMount in https://reactjs.org/docs/portals.html#event-bubbling-through-portals
+        if (typeof document === "undefined" || !this.state.hasMounted) {
+            return null;
+        } else {
+            return ReactDOM.createPortal(this.props.children, this.portalElement);
+        }
     }
 
     public componentDidMount() {
-        document.body.appendChild(this.targetElement);
+        this.portalElement = this.createContainerElement();
+        document.body.appendChild(this.portalElement);
         safeInvoke(this.props.onChildrenMount);
         this.setState({ hasMounted: true });
     }
 
+    public componentDidUpdate(prevProps: IPortalProps) {
+        // update className prop on portal DOM element
+        if (this.portalElement != null && prevProps.className !== this.props.className) {
+            this.portalElement.classList.remove(prevProps.className);
+            maybeAddClass(this.portalElement.classList, this.props.className);
+        }
+    }
+
     public componentWillUnmount() {
-        this.targetElement.remove();
+        if (this.portalElement != null) {
+            this.portalElement.remove();
+        }
+    }
+
+    private createContainerElement() {
+        const container = document.createElement("div");
+        container.classList.add(Classes.PORTAL);
+        maybeAddClass(container.classList, this.props.className);
+        if (this.context != null) {
+            maybeAddClass(container.classList, this.context.blueprintPortalClassName);
+        }
+        return container;
+    }
+}
+
+function maybeAddClass(classList: DOMTokenList, className?: string) {
+    if (className != null && className !== "") {
+        classList.add(className);
     }
 }
diff --git a/packages/core/test/isotest.js b/packages/core/test/isotest.js
@@ -32,10 +32,6 @@ const customChildren = {
     Toaster: React.createElement(Core.Toast, { message: "Toast" }),
 };
 
-const skipList = [
-    "Portal", // doesn't render any DOM inline
-]
-
 describe("Core isomorphic rendering", () => {
-    generateIsomorphicTests(Core, customProps, customChildren, skipList);
+    generateIsomorphicTests(Core, customProps, customChildren);
 });
diff --git a/packages/core/test/portal/portalTests.tsx b/packages/core/test/portal/portalTests.tsx
@@ -30,16 +30,27 @@ describe("<Portal>", () => {
         assert.lengthOf(document.getElementsByClassName(CLASS_TO_TEST), 1);
     });
 
-    it("propagates class names", () => {
+    it("propagates className to portal element", () => {
         const CLASS_TO_TEST = "bp-test-klass";
         portal = mount(
             <Portal className={CLASS_TO_TEST}>
                 <p>test</p>
             </Portal>,
         );
 
-        const portalChild = document.querySelector(`.${CLASS_TO_TEST}`);
-        assert.strictEqual(portalChild.parentElement.className, Classes.PORTAL);
+        const portalChild = document.querySelector(`.${Classes.PORTAL}.${CLASS_TO_TEST}`);
+        assert.exists(portalChild);
+    });
+
+    it("updates className on portal element", () => {
+        portal = mount(
+            <Portal className="class-one">
+                <p>test</p>
+            </Portal>,
+        );
+        assert.exists(portal.find(".class-one"));
+        portal.setProps({ className: "class-two" });
+        assert.exists(portal.find(".class-two"));
     });
 
     it("respects blueprintPortalClassName on context", () => {