From e7e846300d16f28425010c38a35992eb7ec68cc9 Mon Sep 17 00:00:00 2001 From: Brie Bunge Date: Tue, 19 Sep 2017 17:52:12 -0700 Subject: [PATCH 1/4] Ease Popover2 migration with prop shims --- packages/labs/src/common/errors.ts | 13 +++ .../labs/src/components/popover/popover2.tsx | 80 ++++++++++++++++--- .../components/popover/popoverMigrations.ts | 57 +++++++++++++ 3 files changed, 140 insertions(+), 10 deletions(-) create mode 100644 packages/labs/src/common/errors.ts create mode 100644 packages/labs/src/components/popover/popoverMigrations.ts diff --git a/packages/labs/src/common/errors.ts b/packages/labs/src/common/errors.ts new file mode 100644 index 0000000000..596116f641 --- /dev/null +++ b/packages/labs/src/common/errors.ts @@ -0,0 +1,13 @@ +/* + * Copyright 2015 Palantir Technologies, Inc. All rights reserved. + * Licensed under the BSD-3 License as modified (the “License”); you may obtain a copy + * of the license at https://github.com/palantir/blueprint/blob/master/LICENSE + * and https://github.com/palantir/blueprint/blob/master/PATENTS + */ + +const ns = "[Blueprint]"; +const deprec = `${ns} DEPRECATION:`; + +export const POPOVER_WARN_DEPRECATED_IS_DISABLED = `${deprec} isDisabled is deprecated. Use disabled.`; +export const POPOVER_WARN_DEPRECATED_IS_MODAL = `${deprec} isModal is deprecated. Use hasBackdrop.`; +export const POPOVER_WARN_DEPRECATED_POSITION = `${deprec} position is deprecated. Use placement.`; diff --git a/packages/labs/src/components/popover/popover2.tsx b/packages/labs/src/components/popover/popover2.tsx index 1d03cdf781..1d55a7fc1f 100644 --- a/packages/labs/src/components/popover/popover2.tsx +++ b/packages/labs/src/components/popover/popover2.tsx @@ -20,11 +20,14 @@ import { IProps, Overlay, PopoverInteractionKind, + Position, Utils, } from "@blueprintjs/core"; +import * as Errors from "../../common/errors"; import { Tooltip2 } from "../tooltip/tooltip2"; import { getArrowAngle, PopoverArrow } from "./arrow"; +import { migratePosition, resolvePropValue } from "./popoverMigrations"; import { arrowOffsetModifier, getTransformOrigin } from "./popperUtils"; export interface IPopover2Props extends IOverlayableProps, IProps { @@ -79,6 +82,12 @@ export interface IPopover2Props extends IOverlayableProps, IProps { */ disabled?: boolean; + /** + * Prevents the popover from appearing when `true`. + * @deprecated use `disabled` + */ + isDisabled?: boolean; + /** * Enables an invisible overlay beneath the popover that captures clicks and prevents * interaction with the rest of the document until the popover is closed. @@ -88,6 +97,13 @@ export interface IPopover2Props extends IOverlayableProps, IProps { */ hasBackdrop?: boolean; + /** + * Enables an invisible overlay beneath the popover that captures clicks and prevents + * interaction with the rest of the document until the popover is closed. + * @deprecated use `hasBackdrop` + */ + isModal?: boolean; + /** * Whether the popover is visible. Passing this prop puts the popover in * controlled mode, where the only way to change visibility is by updating this property. @@ -122,6 +138,12 @@ export interface IPopover2Props extends IOverlayableProps, IProps { */ openOnTargetFocus?: boolean; + /** + * The position (relative to the target) at which the popover should appear. + * @deprecated use `placement` + */ + position?: Position; + /** * A space-delimited string of class names that are applied to the popover (but not the target). */ @@ -182,8 +204,6 @@ export class Popover2 extends AbstractComponent public static defaultProps: IPopover2Props = { defaultIsOpen: false, - disabled: false, - hasBackdrop: false, hoverCloseDelay: 300, hoverOpenDelay: 150, inheritDarkTheme: true, @@ -192,9 +212,12 @@ export class Popover2 extends AbstractComponent minimal: false, modifiers: {}, openOnTargetFocus: true, - placement: "auto", rootElementTag: "span", transitionDuration: 300, + // Migrated props excluded from default props: + // For `disabled`, see `getDisabled`. + // For `hasBackdrop`, see `getHasBackdrop`. + // For `placement`, see `getPlacement`. }; /** @@ -218,7 +241,8 @@ export class Popover2 extends AbstractComponent public constructor(props?: IPopover2Props, context?: any) { super(props, context); - let isOpen = props.defaultIsOpen && !props.disabled; + const disabled = getDisabled(props); + let isOpen = props.defaultIsOpen && !disabled; if (props.isOpen != null) { isOpen = props.isOpen; } @@ -232,6 +256,7 @@ export class Popover2 extends AbstractComponent public render() { const { className } = this.props; const { isOpen } = this.state; + const disabled = getDisabled(this.props); let targetProps: React.HTMLAttributes; if (this.isHoverInteractionKind()) { @@ -264,7 +289,7 @@ export class Popover2 extends AbstractComponent }); const isContentEmpty = children.content == null; - if (isContentEmpty && !this.props.disabled && isOpen !== false && !Utils.isNodeEnv("production")) { + if (isContentEmpty && !disabled && isOpen !== false && !Utils.isNodeEnv("production")) { console.warn("[Blueprint] Disabling with empty/whitespace content..."); } @@ -282,7 +307,7 @@ export class Popover2 extends AbstractComponent className={this.props.portalClassName} didOpen={this.handleContentMount} enforceFocus={this.props.enforceFocus} - hasBackdrop={this.props.hasBackdrop} + hasBackdrop={getHasBackdrop(this.props)} inline={this.props.inline} isOpen={isOpen && !isContentEmpty} onClose={this.handleOverlayClose} @@ -302,7 +327,10 @@ export class Popover2 extends AbstractComponent public componentWillReceiveProps(nextProps: IPopover2Props) { super.componentWillReceiveProps(nextProps); - if (nextProps.isOpen == null && nextProps.disabled && !this.props.disabled) { + const disabled = getDisabled(this.props); + const nextDisabled = getDisabled(nextProps); + + if (nextProps.isOpen == null && nextDisabled && !disabled) { // ok to use setOpenState here because disabled and isOpen are mutex. this.setOpenState(false); } else if (nextProps.isOpen !== this.props.isOpen) { @@ -329,6 +357,18 @@ export class Popover2 extends AbstractComponent super.componentWillUnmount(); } + protected validateProps(props: IPopover2Props & { children?: React.ReactNode }) { + if (props.isDisabled !== undefined) { + console.warn(Errors.POPOVER_WARN_DEPRECATED_IS_DISABLED); + } + if (props.isModal !== undefined) { + console.warn(Errors.POPOVER_WARN_DEPRECATED_IS_MODAL); + } + if (props.position !== undefined) { + console.warn(Errors.POPOVER_WARN_DEPRECATED_POSITION); + } + } + private updateDarkParent() { if (!this.props.inline) { const hasDarkParent = this.targetElement.closest(`.${Classes.DARK}`) != null; @@ -338,6 +378,8 @@ export class Popover2 extends AbstractComponent private renderPopper(content: JSX.Element) { const { inline, interactionKind, modifiers } = this.props; + const placement = getPlacement(this.props); + const popoverHandlers: React.HTMLAttributes = { // always check popover clicks for dismiss class onClick: this.handlePopoverClick, @@ -378,7 +420,7 @@ export class Popover2 extends AbstractComponent }; return ( - +
!this.props.openOnTargetFocus ) { this.handleMouseLeave(e); - } else if (!this.props.disabled) { + } else if (!getDisabled(this.props)) { // only begin opening popover when it is enabled this.setOpenState(true, e, this.props.hoverOpenDelay); } @@ -470,7 +512,7 @@ export class Popover2 extends AbstractComponent private handleTargetClick = (e: React.MouseEvent) => { // ensure click did not originate from within inline popover before closing - if (!this.props.disabled && !this.isElementInPopover(e.target as HTMLElement)) { + if (!getDisabled(this.props) && !this.isElementInPopover(e.target as HTMLElement)) { if (this.props.isOpen == null) { this.setState(prevState => ({ isOpen: !prevState.isOpen })); } else { @@ -535,3 +577,21 @@ function ensureElement(child: React.ReactChild | undefined) { return child; } } + +function getDisabled(props: IPopover2Props): boolean { + return resolvePropValue([props.disabled, props.isDisabled], false); +} + +function getHasBackdrop(props: IPopover2Props): boolean { + return resolvePropValue([props.hasBackdrop, props.isModal], false); +} + +function getPlacement(props: IPopover2Props): Placement { + let placement: Placement = "auto"; + if (props.placement !== undefined) { + placement = props.placement; + } else if (props.position !== undefined) { + placement = migratePosition(props.position); + } + return placement; +} diff --git a/packages/labs/src/components/popover/popoverMigrations.ts b/packages/labs/src/components/popover/popoverMigrations.ts new file mode 100644 index 0000000000..0f64435232 --- /dev/null +++ b/packages/labs/src/components/popover/popoverMigrations.ts @@ -0,0 +1,57 @@ +/* + * Copyright 2017 Palantir Technologies, Inc. All rights reserved. + * Licensed under the BSD-3 License as modified (the “License”); you may obtain a copy + * of the license at https://github.com/palantir/blueprint/blob/master/LICENSE + * and https://github.com/palantir/blueprint/blob/master/PATENTS + */ + +import { Position } from "@blueprintjs/core"; +import { Placement } from "./popover2"; + +/** + * Get the first non-undefined prop value, falling back to the specified default value. + * @param candidates the ordered list of value candidates + * @param defaultValue the fallback value + */ +export function resolvePropValue(candidates: Array, defaultValue: T): T { + return [...candidates].reverse().reduce((prev, cur) => (cur !== undefined ? cur : prev), defaultValue); +} + +/** + * Migrate a legacy position into a placement. + * @param position the position to convert + */ +export function migratePosition(position: Position): Placement { + switch (position) { + case Position.TOP_LEFT: + return "top-start"; + case Position.TOP: + return "top"; + case Position.TOP_RIGHT: + return "top-end"; + case Position.RIGHT_TOP: + return "right-start"; + case Position.RIGHT: + return "right"; + case Position.RIGHT_BOTTOM: + return "right-end"; + case Position.BOTTOM_RIGHT: + return "bottom-end"; + case Position.BOTTOM: + return "bottom"; + case Position.BOTTOM_LEFT: + return "bottom-start"; + case Position.LEFT_BOTTOM: + return "left-end"; + case Position.LEFT: + return "left"; + case Position.LEFT_TOP: + return "left-start"; + default: + return assertNever(position); + } +} + +function assertNever(x: never): never { + throw new Error("Unexpected position: " + x); +} From 2226a834ae9936467c49f13ce6b2f342e7fc7ee9 Mon Sep 17 00:00:00 2001 From: Brie Bunge Date: Tue, 19 Sep 2017 17:52:18 -0700 Subject: [PATCH 2/4] Add tests --- packages/labs/test/popover2Tests.tsx | 98 +++++++++++++++++++++++++++- 1 file changed, 95 insertions(+), 3 deletions(-) diff --git a/packages/labs/test/popover2Tests.tsx b/packages/labs/test/popover2Tests.tsx index db9d4c5b7c..f1a228ff35 100644 --- a/packages/labs/test/popover2Tests.tsx +++ b/packages/labs/test/popover2Tests.tsx @@ -8,13 +8,14 @@ import { assert } from "chai"; import { mount, ReactWrapper, shallow } from "enzyme"; import * as React from "react"; +import { Popper } from "react-popper"; -import { Classes, Keys, Overlay, PopoverInteractionKind, Tooltip, Utils } from "@blueprintjs/core"; +import { Classes, Keys, Overlay, PopoverInteractionKind, Position, Tooltip, Utils } from "@blueprintjs/core"; import * as Errors from "@blueprintjs/core/src/common/errors"; import { dispatchMouseEvent } from "@blueprintjs/core/test/common/utils"; import { Arrow } from "react-popper"; -import { IPopover2Props, IPopover2State, Popover2 } from "../src/index"; +import { IPopover2Props, IPopover2State, Placement, Popover2 } from "../src/index"; describe("", () => { let testsContainerElement: HTMLElement; @@ -72,7 +73,7 @@ describe("", () => { }); }); - it("propogates class names correctly", () => { + it("propagates class names correctly", () => { wrapper = renderPopover({ className: "bar", interactionKind: PopoverInteractionKind.CLICK_TARGET_ONLY, @@ -576,6 +577,97 @@ describe("", () => { }); }); + describe("deprecated prop shims", () => { + it("should convert position to placement", () => { + const popover = shallow( + + child + , + ); + { + const actual: Placement = popover.find(Popper).prop("placement"); + const expected: Placement = "bottom-start"; + assert.strictEqual(actual, expected); + } + + popover.setProps({ position: Position.LEFT_BOTTOM }); + { + const actual: Placement = popover.find(Popper).prop("placement"); + const expected: Placement = "left-end"; + assert.strictEqual(actual, expected); + } + }); + + it("should convert isModal to hasBackdrop", () => { + const popover = shallow( + + child + , + ); + assert.isTrue(popover.find(Overlay).prop("hasBackdrop")); + + popover.setProps({ isModal: false }); + assert.isFalse(popover.find(Overlay).prop("hasBackdrop")); + }); + + it("should convert isDisabled to disabled", () => { + renderPopover({ + interactionKind: PopoverInteractionKind.CLICK_TARGET_ONLY, + isDisabled: true, + }) + .simulateTarget("click") + .assertIsOpen(false) + .setProps({ isDisabled: false }) + .simulateTarget("click") + .assertIsOpen(true); + }); + + it("placement should take precedence over position", () => { + const popover = shallow( + + child + , + ); + { + const actual: Placement = popover.find(Popper).prop("placement"); + const expected: Placement = "left-end"; + assert.strictEqual(actual, expected); + } + + popover.setProps({ placement: "bottom-start", position: Position.LEFT_BOTTOM }); + { + const actual: Placement = popover.find(Popper).prop("placement"); + const expected: Placement = "bottom-start"; + assert.strictEqual(actual, expected); + } + }); + + it("hasBackdrop should take precedence over isModal", () => { + const popover = shallow( + + child + , + ); + assert.isTrue(popover.find(Overlay).prop("hasBackdrop")); + + popover.setProps({ hasBackdrop: false, isModal: true }); + assert.isFalse(popover.find(Overlay).prop("hasBackdrop")); + }); + + it("disabled should take precedence over isDisabled", () => { + renderPopover({ + disabled: true, + interactionKind: PopoverInteractionKind.CLICK_TARGET_ONLY, + isDisabled: false, + }) + .simulateTarget("click") + .assertIsOpen(false) + .setProps({ disabled: false, isDisabled: true }) + .simulateTarget("click") + .assertIsOpen(true); + }); + }); + interface IPopoverWrapper extends ReactWrapper { popover: HTMLElement; assertIsOpen(isOpen?: boolean): this; From bd667795bea6829f161ce330568c01696337ed3d Mon Sep 17 00:00:00 2001 From: Brie Bunge Date: Fri, 22 Sep 2017 11:45:31 -0700 Subject: [PATCH 3/4] Address PR feedback --- packages/labs/src/common/errors.ts | 6 ++-- .../labs/src/components/popover/popover2.tsx | 10 +++--- ...Migrations.ts => popoverMigrationUtils.ts} | 13 ++++++-- packages/labs/test/popover2Tests.tsx | 32 +++++++------------ 4 files changed, 29 insertions(+), 32 deletions(-) rename packages/labs/src/components/popover/{popoverMigrations.ts => popoverMigrationUtils.ts} (83%) diff --git a/packages/labs/src/common/errors.ts b/packages/labs/src/common/errors.ts index 596116f641..9ea680374d 100644 --- a/packages/labs/src/common/errors.ts +++ b/packages/labs/src/common/errors.ts @@ -8,6 +8,6 @@ const ns = "[Blueprint]"; const deprec = `${ns} DEPRECATION:`; -export const POPOVER_WARN_DEPRECATED_IS_DISABLED = `${deprec} isDisabled is deprecated. Use disabled.`; -export const POPOVER_WARN_DEPRECATED_IS_MODAL = `${deprec} isModal is deprecated. Use hasBackdrop.`; -export const POPOVER_WARN_DEPRECATED_POSITION = `${deprec} position is deprecated. Use placement.`; +export const POPOVER2_WARN_DEPRECATED_IS_DISABLED = `${deprec} isDisabled is deprecated. Use disabled.`; +export const POPOVER2_WARN_DEPRECATED_IS_MODAL = `${deprec} isModal is deprecated. Use hasBackdrop.`; +export const POPOVER2_WARN_DEPRECATED_POSITION = `${deprec} position is deprecated. Use placement.`; diff --git a/packages/labs/src/components/popover/popover2.tsx b/packages/labs/src/components/popover/popover2.tsx index 1d55a7fc1f..ec6a3a86a0 100644 --- a/packages/labs/src/components/popover/popover2.tsx +++ b/packages/labs/src/components/popover/popover2.tsx @@ -27,7 +27,7 @@ import { import * as Errors from "../../common/errors"; import { Tooltip2 } from "../tooltip/tooltip2"; import { getArrowAngle, PopoverArrow } from "./arrow"; -import { migratePosition, resolvePropValue } from "./popoverMigrations"; +import { positionToPlacement, resolvePropValue } from "./popoverMigrationUtils"; import { arrowOffsetModifier, getTransformOrigin } from "./popperUtils"; export interface IPopover2Props extends IOverlayableProps, IProps { @@ -359,13 +359,13 @@ export class Popover2 extends AbstractComponent protected validateProps(props: IPopover2Props & { children?: React.ReactNode }) { if (props.isDisabled !== undefined) { - console.warn(Errors.POPOVER_WARN_DEPRECATED_IS_DISABLED); + console.warn(Errors.POPOVER2_WARN_DEPRECATED_IS_DISABLED); } if (props.isModal !== undefined) { - console.warn(Errors.POPOVER_WARN_DEPRECATED_IS_MODAL); + console.warn(Errors.POPOVER2_WARN_DEPRECATED_IS_MODAL); } if (props.position !== undefined) { - console.warn(Errors.POPOVER_WARN_DEPRECATED_POSITION); + console.warn(Errors.POPOVER2_WARN_DEPRECATED_POSITION); } } @@ -591,7 +591,7 @@ function getPlacement(props: IPopover2Props): Placement { if (props.placement !== undefined) { placement = props.placement; } else if (props.position !== undefined) { - placement = migratePosition(props.position); + placement = positionToPlacement(props.position); } return placement; } diff --git a/packages/labs/src/components/popover/popoverMigrations.ts b/packages/labs/src/components/popover/popoverMigrationUtils.ts similarity index 83% rename from packages/labs/src/components/popover/popoverMigrations.ts rename to packages/labs/src/components/popover/popoverMigrationUtils.ts index 0f64435232..096c7f17eb 100644 --- a/packages/labs/src/components/popover/popoverMigrations.ts +++ b/packages/labs/src/components/popover/popoverMigrationUtils.ts @@ -14,14 +14,21 @@ import { Placement } from "./popover2"; * @param defaultValue the fallback value */ export function resolvePropValue(candidates: Array, defaultValue: T): T { - return [...candidates].reverse().reduce((prev, cur) => (cur !== undefined ? cur : prev), defaultValue); + let value: T | undefined; + for (const candidate of candidates) { + if (candidate !== undefined) { + value = candidate; + break; + } + } + return value !== undefined ? value : defaultValue; } /** - * Migrate a legacy position into a placement. + * Convert a position to a placement. * @param position the position to convert */ -export function migratePosition(position: Position): Placement { +export function positionToPlacement(position: Position): Placement { switch (position) { case Position.TOP_LEFT: return "top-start"; diff --git a/packages/labs/test/popover2Tests.tsx b/packages/labs/test/popover2Tests.tsx index f1a228ff35..f75edbb3a6 100644 --- a/packages/labs/test/popover2Tests.tsx +++ b/packages/labs/test/popover2Tests.tsx @@ -6,7 +6,7 @@ */ import { assert } from "chai"; -import { mount, ReactWrapper, shallow } from "enzyme"; +import { mount, ReactWrapper, shallow, ShallowWrapper } from "enzyme"; import * as React from "react"; import { Popper } from "react-popper"; @@ -17,6 +17,8 @@ import { dispatchMouseEvent } from "@blueprintjs/core/test/common/utils"; import { Arrow } from "react-popper"; import { IPopover2Props, IPopover2State, Placement, Popover2 } from "../src/index"; +type ShallowPopover2Wrapper = ShallowWrapper; + describe("", () => { let testsContainerElement: HTMLElement; let wrapper: IPopoverWrapper; @@ -584,18 +586,10 @@ describe("", () => { child , ); - { - const actual: Placement = popover.find(Popper).prop("placement"); - const expected: Placement = "bottom-start"; - assert.strictEqual(actual, expected); - } + assertPlacement(popover, "bottom-start"); popover.setProps({ position: Position.LEFT_BOTTOM }); - { - const actual: Placement = popover.find(Popper).prop("placement"); - const expected: Placement = "left-end"; - assert.strictEqual(actual, expected); - } + assertPlacement(popover, "left-end"); }); it("should convert isModal to hasBackdrop", () => { @@ -628,18 +622,10 @@ describe("", () => { child , ); - { - const actual: Placement = popover.find(Popper).prop("placement"); - const expected: Placement = "left-end"; - assert.strictEqual(actual, expected); - } + assertPlacement(popover, "left-end"); popover.setProps({ placement: "bottom-start", position: Position.LEFT_BOTTOM }); - { - const actual: Placement = popover.find(Popper).prop("placement"); - const expected: Placement = "bottom-start"; - assert.strictEqual(actual, expected); - } + assertPlacement(popover, "bottom-start"); }); it("hasBackdrop should take precedence over isModal", () => { @@ -714,4 +700,8 @@ describe("", () => { function getNode(element: ReactWrapper, any>) { return (element as any).node as Element; } + + function assertPlacement(popover: ShallowPopover2Wrapper, placement: Placement) { + assert.strictEqual(popover.find(Popper).prop("placement"), placement); + } }); From 5a1a5c662018b2f0dfbdafc83b93164df98d2dd4 Mon Sep 17 00:00:00 2001 From: Brie Bunge Date: Fri, 22 Sep 2017 12:26:05 -0700 Subject: [PATCH 4/4] Use state for derived migrated prop values --- .../labs/src/components/popover/popover2.tsx | 62 +++++++++++++------ .../popover/popoverMigrationUtils.ts | 16 ----- 2 files changed, 43 insertions(+), 35 deletions(-) diff --git a/packages/labs/src/components/popover/popover2.tsx b/packages/labs/src/components/popover/popover2.tsx index ec6a3a86a0..0cc3c83937 100644 --- a/packages/labs/src/components/popover/popover2.tsx +++ b/packages/labs/src/components/popover/popover2.tsx @@ -27,7 +27,7 @@ import { import * as Errors from "../../common/errors"; import { Tooltip2 } from "../tooltip/tooltip2"; import { getArrowAngle, PopoverArrow } from "./arrow"; -import { positionToPlacement, resolvePropValue } from "./popoverMigrationUtils"; +import { positionToPlacement } from "./popoverMigrationUtils"; import { arrowOffsetModifier, getTransformOrigin } from "./popperUtils"; export interface IPopover2Props extends IOverlayableProps, IProps { @@ -196,6 +196,15 @@ export interface IPopover2State { transformOrigin?: string; isOpen?: boolean; hasDarkParent?: boolean; + + /** Migrated `disabled` value that considers the `disabled` and `isDisabled` props. */ + disabled?: boolean; + + /** Migrated `hasBackdrop` value that considers the `hasBackdrop` and `isModal` props. */ + hasBackdrop?: boolean; + + /** Migrated `placement` value that considers the `placement` and `position` props. */ + placement?: Placement; } @PureRender @@ -214,10 +223,6 @@ export class Popover2 extends AbstractComponent openOnTargetFocus: true, rootElementTag: "span", transitionDuration: 300, - // Migrated props excluded from default props: - // For `disabled`, see `getDisabled`. - // For `hasBackdrop`, see `getHasBackdrop`. - // For `placement`, see `getPlacement`. }; /** @@ -248,15 +253,17 @@ export class Popover2 extends AbstractComponent } this.state = { + disabled, + hasBackdrop: getHasBackdrop(props), hasDarkParent: false, isOpen, + placement: getPlacement(props), }; } public render() { const { className } = this.props; - const { isOpen } = this.state; - const disabled = getDisabled(this.props); + const { isOpen, disabled, hasBackdrop } = this.state; let targetProps: React.HTMLAttributes; if (this.isHoverInteractionKind()) { @@ -307,7 +314,7 @@ export class Popover2 extends AbstractComponent className={this.props.portalClassName} didOpen={this.handleContentMount} enforceFocus={this.props.enforceFocus} - hasBackdrop={getHasBackdrop(this.props)} + hasBackdrop={hasBackdrop} inline={this.props.inline} isOpen={isOpen && !isContentEmpty} onClose={this.handleOverlayClose} @@ -327,10 +334,9 @@ export class Popover2 extends AbstractComponent public componentWillReceiveProps(nextProps: IPopover2Props) { super.componentWillReceiveProps(nextProps); - const disabled = getDisabled(this.props); const nextDisabled = getDisabled(nextProps); - if (nextProps.isOpen == null && nextDisabled && !disabled) { + if (nextProps.isOpen == null && nextDisabled && !this.state.disabled) { // ok to use setOpenState here because disabled and isOpen are mutex. this.setOpenState(false); } else if (nextProps.isOpen !== this.props.isOpen) { @@ -338,6 +344,12 @@ export class Popover2 extends AbstractComponent // (which would be invoked if this went through setOpenState) this.setState({ isOpen: nextProps.isOpen }); } + + this.setState({ + disabled: nextDisabled, + hasBackdrop: getHasBackdrop(nextProps), + placement: getPlacement(nextProps), + }); } public componentWillUpdate(_: IPopover2Props, nextState: IPopover2State) { @@ -378,7 +390,7 @@ export class Popover2 extends AbstractComponent private renderPopper(content: JSX.Element) { const { inline, interactionKind, modifiers } = this.props; - const placement = getPlacement(this.props); + const { placement } = this.state; const popoverHandlers: React.HTMLAttributes = { // always check popover clicks for dismiss class @@ -482,7 +494,7 @@ export class Popover2 extends AbstractComponent !this.props.openOnTargetFocus ) { this.handleMouseLeave(e); - } else if (!getDisabled(this.props)) { + } else if (!this.state.disabled) { // only begin opening popover when it is enabled this.setOpenState(true, e, this.props.hoverOpenDelay); } @@ -512,7 +524,7 @@ export class Popover2 extends AbstractComponent private handleTargetClick = (e: React.MouseEvent) => { // ensure click did not originate from within inline popover before closing - if (!getDisabled(this.props) && !this.isElementInPopover(e.target as HTMLElement)) { + if (!this.state.disabled && !this.isElementInPopover(e.target as HTMLElement)) { if (this.props.isOpen == null) { this.setState(prevState => ({ isOpen: !prevState.isOpen })); } else { @@ -579,19 +591,31 @@ function ensureElement(child: React.ReactChild | undefined) { } function getDisabled(props: IPopover2Props): boolean { - return resolvePropValue([props.disabled, props.isDisabled], false); + if (props.disabled !== undefined) { + return props.disabled; + } else if (props.isDisabled !== undefined) { + return props.isDisabled; + } else { + return false; + } } function getHasBackdrop(props: IPopover2Props): boolean { - return resolvePropValue([props.hasBackdrop, props.isModal], false); + if (props.hasBackdrop !== undefined) { + return props.hasBackdrop; + } else if (props.isModal !== undefined) { + return props.isModal; + } else { + return false; + } } function getPlacement(props: IPopover2Props): Placement { - let placement: Placement = "auto"; if (props.placement !== undefined) { - placement = props.placement; + return props.placement; } else if (props.position !== undefined) { - placement = positionToPlacement(props.position); + return positionToPlacement(props.position); + } else { + return "auto"; } - return placement; } diff --git a/packages/labs/src/components/popover/popoverMigrationUtils.ts b/packages/labs/src/components/popover/popoverMigrationUtils.ts index 096c7f17eb..44e8131248 100644 --- a/packages/labs/src/components/popover/popoverMigrationUtils.ts +++ b/packages/labs/src/components/popover/popoverMigrationUtils.ts @@ -8,22 +8,6 @@ import { Position } from "@blueprintjs/core"; import { Placement } from "./popover2"; -/** - * Get the first non-undefined prop value, falling back to the specified default value. - * @param candidates the ordered list of value candidates - * @param defaultValue the fallback value - */ -export function resolvePropValue(candidates: Array, defaultValue: T): T { - let value: T | undefined; - for (const candidate of candidates) { - if (candidate !== undefined) { - value = candidate; - break; - } - } - return value !== undefined ? value : defaultValue; -} - /** * Convert a position to a placement. * @param position the position to convert