From d72183384ba974dd3c6dae7a41ca5eda2a0d3ece Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 6 Mar 2018 11:33:15 -0800 Subject: [PATCH 1/6] refactor Portal to support SSR. - created DOM element receives className - remove containerRef prop and associated element --- .../core/src/components/portal/portal.tsx | 61 ++++++++++--------- packages/core/test/portal/portalTests.tsx | 17 +++++- 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/packages/core/src/components/portal/portal.tsx b/packages/core/src/components/portal/portal.tsx index e6a7570669c..c6d60311495 100644 --- 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 { - /** - * 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,44 @@ export class Portal extends React.Component { 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( -
- {this.state.hasMounted ? this.props.children : null} -
, - 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 + return typeof document !== "undefined" && this.state.hasMounted + ? ReactDOM.createPortal(this.props.children, this.portalElement) + : null; } public componentDidMount() { - document.body.appendChild(this.targetElement); + this.portalElement = this.createElement(); + 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); + this.portalElement.classList.add(this.props.className); + } + } + public componentWillUnmount() { - this.targetElement.remove(); + if (this.portalElement != null) { + this.portalElement.remove(); + } + } + + private createElement() { + const container = document.createElement("div"); + container.classList.add(Classes.PORTAL, this.props.className); + if (this.context && this.context.blueprintPortalClassName != null) { + container.classList.add(this.context.blueprintPortalClassName); + } + return container; } } diff --git a/packages/core/test/portal/portalTests.tsx b/packages/core/test/portal/portalTests.tsx index c728ddd1d77..40c14516660 100644 --- a/packages/core/test/portal/portalTests.tsx +++ b/packages/core/test/portal/portalTests.tsx @@ -30,7 +30,7 @@ describe("", () => { 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( @@ -38,8 +38,19 @@ describe("", () => { , ); - 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( + +

test

+ , + ); + assert.exists(portal.find(".class-one")); + portal.setProps({ className: "class-two" }); + assert.exists(portal.find(".class-two")); }); it("respects blueprintPortalClassName on context", () => { From 3fcf864ea1fa2d1ce4dc91430294cae4b3bfb19c Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 6 Mar 2018 11:40:52 -0800 Subject: [PATCH 2/6] refactor Overlay to not use deleted containerRef prop - use TransitionGroup as the wrapper element!! --- .../core/src/components/overlay/overlay.tsx | 34 ++++++------------- 1 file changed, 11 insertions(+), 23 deletions(-) diff --git a/packages/core/src/components/overlay/overlay.tsx b/packages/core/src/components/overlay/overlay.tsx index d3e7ef4fe18..38378e60a3d 100644 --- 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 { // 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) { @@ -178,13 +179,6 @@ export class Overlay extends React.PureComponent { ); }); - const transitionGroup = ( - - {this.maybeRenderBackdrop()} - {isOpen ? childrenWithTransitions : null} - - ); - const mergedClassName = classNames( Classes.OVERLAY, { @@ -199,22 +193,16 @@ export class Overlay extends React.PureComponent { onKeyDown: this.handleKeyDown, }; + const transitionGroup = ( + + {this.maybeRenderBackdrop()} + {isOpen ? childrenWithTransitions : null} + + ); if (usePortal) { - return ( - - {transitionGroup} - - ); + return {transitionGroup}; } else { - return ( - - {transitionGroup} - - ); + return transitionGroup; } } @@ -289,7 +277,7 @@ export class Overlay extends React.PureComponent { ); } else { - return undefined; + return null; } } From 2a1057a17c71410096b49622567f7a892342608b Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 6 Mar 2018 11:41:02 -0800 Subject: [PATCH 3/6] conditionally add classes in portal --- packages/core/src/components/portal/portal.tsx | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/core/src/components/portal/portal.tsx b/packages/core/src/components/portal/portal.tsx index c6d60311495..d5fe53b6bfb 100644 --- a/packages/core/src/components/portal/portal.tsx +++ b/packages/core/src/components/portal/portal.tsx @@ -71,7 +71,7 @@ export class Portal extends React.Component { // update className prop on portal DOM element if (this.portalElement != null && prevProps.className !== this.props.className) { this.portalElement.classList.remove(prevProps.className); - this.portalElement.classList.add(this.props.className); + maybeAddClass(this.portalElement.classList, this.props.className); } } @@ -83,10 +83,17 @@ export class Portal extends React.Component { private createElement() { const container = document.createElement("div"); - container.classList.add(Classes.PORTAL, this.props.className); - if (this.context && this.context.blueprintPortalClassName != null) { - container.classList.add(this.context.blueprintPortalClassName); + 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); + } +} From 01b2e1b5fecf55b34f2a381a32159b534e639dda Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 6 Mar 2018 11:41:39 -0800 Subject: [PATCH 4/6] enable Portal isotest --- packages/core/test/isotest.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/core/test/isotest.js b/packages/core/test/isotest.js index 21f7d31bb8f..6e0ee4b1507 100644 --- 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); }); From 857fc02239c249b19b690be0b3fbe3899a59c51e Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 6 Mar 2018 12:06:50 -0800 Subject: [PATCH 5/6] refactor TransitionGroup usage --- .../core/src/components/overlay/overlay.tsx | 65 +++++++++++-------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/packages/core/src/components/overlay/overlay.tsx b/packages/core/src/components/overlay/overlay.tsx index 38378e60a3d..c589cac712e 100644 --- a/packages/core/src/components/overlay/overlay.tsx +++ b/packages/core/src/components/overlay/overlay.tsx @@ -161,25 +161,36 @@ export class Overlay extends React.PureComponent { const { children, className, usePortal, isOpen, transitionDuration, transitionName } = 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 ( - - {decoratedChild} - - ); - }); - - const mergedClassName = classNames( + // 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, (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" ? ( + {child} + ) : ( + React.cloneElement(child, { + className: classNames(child.props.className, Classes.OVERLAY_CONTENT), + tabIndex: 0, + }) + ); + return ( + + {decoratedChild} + + ); + }) + : []; + childrenWithTransitions.unshift(this.maybeRenderBackdrop()); + + const containerClasses = classNames( Classes.OVERLAY, { [Classes.OVERLAY_OPEN]: isOpen, @@ -188,15 +199,15 @@ export class Overlay extends React.PureComponent { className, ); - const elementProps = { - className: mergedClassName, - onKeyDown: this.handleKeyDown, - }; - const transitionGroup = ( - - {this.maybeRenderBackdrop()} - {isOpen ? childrenWithTransitions : null} + + {childrenWithTransitions} ); if (usePortal) { From d47970ecf357614c4db6a7c1ac935a74fd1bc772 Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Wed, 7 Mar 2018 11:52:09 -0800 Subject: [PATCH 6/6] ternary style refactors --- .../core/src/components/overlay/overlay.tsx | 51 ++++++++++--------- .../core/src/components/portal/portal.tsx | 14 ++--- 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/packages/core/src/components/overlay/overlay.tsx b/packages/core/src/components/overlay/overlay.tsx index c589cac712e..3ca46294d6e 100644 --- a/packages/core/src/components/overlay/overlay.tsx +++ b/packages/core/src/components/overlay/overlay.tsx @@ -159,35 +159,12 @@ export class Overlay extends React.PureComponent { return null; } - const { children, className, usePortal, isOpen, transitionDuration, transitionName } = this.props; + const { children, className, usePortal, isOpen } = this.props; // 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, (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" ? ( - {child} - ) : ( - React.cloneElement(child, { - className: classNames(child.props.className, Classes.OVERLAY_CONTENT), - tabIndex: 0, - }) - ); - return ( - - {decoratedChild} - - ); - }) - : []; + const childrenWithTransitions = isOpen ? React.Children.map(children, this.maybeRenderChild) : []; childrenWithTransitions.unshift(this.maybeRenderBackdrop()); const containerClasses = classNames( @@ -266,6 +243,30 @@ export class Overlay extends React.PureComponent { }); } + 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, + }) + ) : ( + {child} + ); + const { transitionDuration, transitionName } = this.props; + return ( + + {decoratedChild} + + ); + }; + private maybeRenderBackdrop() { const { backdropClassName, diff --git a/packages/core/src/components/portal/portal.tsx b/packages/core/src/components/portal/portal.tsx index d5fe53b6bfb..1f3404f9375 100644 --- a/packages/core/src/components/portal/portal.tsx +++ b/packages/core/src/components/portal/portal.tsx @@ -24,7 +24,7 @@ export interface IPortalState { } export interface IPortalContext { - /** Additional CSS classes to add to all `Portal elements in this React context. */ + /** Additional CSS classes to add to all `Portal` elements in this React context. */ blueprintPortalClassName?: string; } @@ -55,13 +55,15 @@ export class Portal extends React.Component { // 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 - return typeof document !== "undefined" && this.state.hasMounted - ? ReactDOM.createPortal(this.props.children, this.portalElement) - : null; + if (typeof document === "undefined" || !this.state.hasMounted) { + return null; + } else { + return ReactDOM.createPortal(this.props.children, this.portalElement); + } } public componentDidMount() { - this.portalElement = this.createElement(); + this.portalElement = this.createContainerElement(); document.body.appendChild(this.portalElement); safeInvoke(this.props.onChildrenMount); this.setState({ hasMounted: true }); @@ -81,7 +83,7 @@ export class Portal extends React.Component { } } - private createElement() { + private createContainerElement() { const container = document.createElement("div"); container.classList.add(Classes.PORTAL); maybeAddClass(container.classList, this.props.className);