From 2b4c3c894d1ba9988fd5464a858aed0e7c9ae393 Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Tue, 24 Jul 2018 12:13:37 +1200 Subject: [PATCH 01/11] Add ResizeObserver to force popper to rerender when content resizes --- packages/core/src/components/popover/popover.tsx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index 270291494f..cb0c8889a3 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -8,6 +8,7 @@ import classNames from "classnames"; import { ModifierFn } from "popper.js"; import * as React from "react"; import { Manager, Popper, PopperChildrenProps, Reference, ReferenceChildrenProps } from "react-popper"; +import ResizeObserver from "resize-observer-polyfill"; import { AbstractPureComponent } from "../../common/abstractPureComponent"; import * as Classes from "../../common/classes"; @@ -127,6 +128,9 @@ export class Popover extends AbstractPureComponent // element on the same page. private lostFocusOnSamePage = true; + // A reference to the Resize observer to prevent it getting recreated on every render + private popperObserver: ResizeObserver; + private refHandlers = { popover: (ref: HTMLElement) => { this.popoverElement = ref; @@ -288,6 +292,12 @@ export class Popover extends AbstractPureComponent this.props.popoverClassName, ); + // If the popover resizes, force popper to rerender. + if (this.popoverElement && !this.popperObserver) { + this.popperObserver = new ResizeObserver(popperProps.scheduleUpdate); + this.popperObserver.observe(this.popoverElement); + } + return (
From 15ae92c9db370c0e5185b7866e233d2a62396946 Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Tue, 24 Jul 2018 13:13:58 +1200 Subject: [PATCH 02/11] Rework observer/resize implmentation, as was only working on intial render --- .../core/src/components/popover/popover.tsx | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index cb0c8889a3..bf8258322d 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -128,9 +128,12 @@ export class Popover extends AbstractPureComponent // element on the same page. private lostFocusOnSamePage = true; - // A reference to the Resize observer to prevent it getting recreated on every render + /** ResizeObserver instance to monitor for content size changes on the popover */ private popperObserver: ResizeObserver; + /** Reference to the Poppper.scheduleUpdate() function, this changes every time the popper is mounted */ + private popperUpdater: () => {}; + private refHandlers = { popover: (ref: HTMLElement) => { this.popoverElement = ref; @@ -205,6 +208,7 @@ export class Popover extends AbstractPureComponent public componentDidMount() { this.updateDarkParent(); + this.popperObserver = new ResizeObserver(() => this.popperUpdater && this.popperUpdater()); } public componentWillReceiveProps(nextProps: IPopoverProps) { @@ -225,6 +229,11 @@ export class Popover extends AbstractPureComponent public componentDidUpdate() { this.updateDarkParent(); + + if (this.popoverElement) { + // Ensure our observer has an up-date-date reference to popoverElement + this.popperObserver.observe(this.popoverElement); + } } public componentWillUnmount() { @@ -292,11 +301,7 @@ export class Popover extends AbstractPureComponent this.props.popoverClassName, ); - // If the popover resizes, force popper to rerender. - if (this.popoverElement && !this.popperObserver) { - this.popperObserver = new ResizeObserver(popperProps.scheduleUpdate); - this.popperObserver.observe(this.popoverElement); - } + this.popperUpdater = popperProps.scheduleUpdate; return (
From 6394932c85c036d3f09964a61ebc10689c4c01d5 Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Tue, 24 Jul 2018 13:33:06 +1200 Subject: [PATCH 03/11] fix typo in comment --- packages/core/src/components/popover/popover.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index bf8258322d..c8f4c68275 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -231,7 +231,7 @@ export class Popover extends AbstractPureComponent this.updateDarkParent(); if (this.popoverElement) { - // Ensure our observer has an up-date-date reference to popoverElement + // Ensure our observer has an up-to-date reference to popoverElement this.popperObserver.observe(this.popoverElement); } } From 94078b7dbc9b8ff2cbe7f61fa386dd8903f808fd Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Tue, 24 Jul 2018 13:39:02 +1200 Subject: [PATCH 04/11] Fixing popperUpdater type definition --- packages/core/src/components/popover/popover.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index c8f4c68275..bcb82ee823 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -132,7 +132,7 @@ export class Popover extends AbstractPureComponent private popperObserver: ResizeObserver; /** Reference to the Poppper.scheduleUpdate() function, this changes every time the popper is mounted */ - private popperUpdater: () => {}; + private popperUpdater: () => void; private refHandlers = { popover: (ref: HTMLElement) => { From 3788f2abec0564fdc6e1448eb950127d166e7ea9 Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Tue, 24 Jul 2018 14:01:36 +1200 Subject: [PATCH 05/11] Align with code style guides --- packages/core/src/components/popover/popover.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index bcb82ee823..8e81e841b5 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -128,10 +128,10 @@ export class Popover extends AbstractPureComponent // element on the same page. private lostFocusOnSamePage = true; - /** ResizeObserver instance to monitor for content size changes on the popover */ + // ResizeObserver instance to monitor for content size changes on the popover private popperObserver: ResizeObserver; - /** Reference to the Poppper.scheduleUpdate() function, this changes every time the popper is mounted */ + // Reference to the Poppper.scheduleUpdate() function, this changes every time the popper is mounted private popperUpdater: () => void; private refHandlers = { @@ -230,7 +230,7 @@ export class Popover extends AbstractPureComponent public componentDidUpdate() { this.updateDarkParent(); - if (this.popoverElement) { + if (this.popoverElement instanceof HTMLElement) { // Ensure our observer has an up-to-date reference to popoverElement this.popperObserver.observe(this.popoverElement); } From ca0de3751f95ec74fcfd365e8794905d186a9060 Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Wed, 25 Jul 2018 09:03:24 +1200 Subject: [PATCH 06/11] Clean up implementation as per review --- .../core/src/components/popover/popover.tsx | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index 8e81e841b5..2c01d7ca0c 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -132,7 +132,7 @@ export class Popover extends AbstractPureComponent private popperObserver: ResizeObserver; // Reference to the Poppper.scheduleUpdate() function, this changes every time the popper is mounted - private popperUpdater: () => void; + private popperScheduleUpdate: () => void; private refHandlers = { popover: (ref: HTMLElement) => { @@ -208,7 +208,7 @@ export class Popover extends AbstractPureComponent public componentDidMount() { this.updateDarkParent(); - this.popperObserver = new ResizeObserver(() => this.popperUpdater && this.popperUpdater()); + this.updatePopperObserver(); } public componentWillReceiveProps(nextProps: IPopoverProps) { @@ -231,6 +231,8 @@ export class Popover extends AbstractPureComponent this.updateDarkParent(); if (this.popoverElement instanceof HTMLElement) { + // Reset the the observer to avoid the list of observed elements growing + this.updatePopperObserver(); // Ensure our observer has an up-to-date reference to popoverElement this.popperObserver.observe(this.popoverElement); } @@ -238,6 +240,7 @@ export class Popover extends AbstractPureComponent public componentWillUnmount() { super.componentWillUnmount(); + this.popperObserver.disconnect(); } protected validateProps(props: IPopoverProps & { children?: React.ReactNode }) { @@ -276,10 +279,22 @@ export class Popover extends AbstractPureComponent } } + private updatePopperObserver() { + + if (this.popperObserver instanceof ResizeObserver) { + this.popperObserver.disconnect(); + } + + this.popperObserver = new ResizeObserver(() => this.popperScheduleUpdate && this.popperScheduleUpdate()); + } + private renderPopover = (popperProps: PopperChildrenProps) => { const { usePortal, interactionKind } = this.props; const { transformOrigin } = this.state; + // Need to update our reference to this on every render as the target node may have changed. + this.popperScheduleUpdate = popperProps.scheduleUpdate; + const popoverHandlers: HTMLDivProps = { // always check popover clicks for dismiss class onClick: this.handlePopoverClick, @@ -301,8 +316,6 @@ export class Popover extends AbstractPureComponent this.props.popoverClassName, ); - this.popperUpdater = popperProps.scheduleUpdate; - return (
From fdfd8a30ba2af9910212ac7e5a9c63df194d00bf Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Wed, 25 Jul 2018 09:15:06 +1200 Subject: [PATCH 07/11] Fixing lint error --- packages/core/src/components/popover/popover.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index 2c01d7ca0c..a144e7fef7 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -280,7 +280,6 @@ export class Popover extends AbstractPureComponent } private updatePopperObserver() { - if (this.popperObserver instanceof ResizeObserver) { this.popperObserver.disconnect(); } From 65108019fee41ccf543c6a8e36c804fa086daa91 Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Wed, 25 Jul 2018 09:31:00 +1200 Subject: [PATCH 08/11] Only initialise observer once, disconnect to clear observations --- packages/core/src/components/popover/popover.tsx | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index a144e7fef7..ea39d6fca5 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -208,7 +208,7 @@ export class Popover extends AbstractPureComponent public componentDidMount() { this.updateDarkParent(); - this.updatePopperObserver(); + this.popperObserver = new ResizeObserver(() => this.popperScheduleUpdate && this.popperScheduleUpdate()); } public componentWillReceiveProps(nextProps: IPopoverProps) { @@ -231,8 +231,9 @@ export class Popover extends AbstractPureComponent this.updateDarkParent(); if (this.popoverElement instanceof HTMLElement) { - // Reset the the observer to avoid the list of observed elements growing - this.updatePopperObserver(); + // Clear active observations to avoid the list growing. + this.popperObserver.disconnect(); + // Ensure our observer has an up-to-date reference to popoverElement this.popperObserver.observe(this.popoverElement); } @@ -279,14 +280,6 @@ export class Popover extends AbstractPureComponent } } - private updatePopperObserver() { - if (this.popperObserver instanceof ResizeObserver) { - this.popperObserver.disconnect(); - } - - this.popperObserver = new ResizeObserver(() => this.popperScheduleUpdate && this.popperScheduleUpdate()); - } - private renderPopover = (popperProps: PopperChildrenProps) => { const { usePortal, interactionKind } = this.props; const { transformOrigin } = this.state; From a9838d8fd2218260c94de2a6c2005a7e8c8bc4a0 Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Wed, 25 Jul 2018 10:02:48 +1200 Subject: [PATCH 09/11] Reword comment --- packages/core/src/components/popover/popover.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index ea39d6fca5..1d9c8ef52c 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -284,7 +284,7 @@ export class Popover extends AbstractPureComponent const { usePortal, interactionKind } = this.props; const { transformOrigin } = this.state; - // Need to update our reference to this on every render as the target node may have changed. + // Need to update our reference to this on every render as it will change. this.popperScheduleUpdate = popperProps.scheduleUpdate; const popoverHandlers: HTMLDivProps = { From 53d42af934424b954f9bdcd50f351116fef0478a Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Wed, 25 Jul 2018 10:35:04 +1200 Subject: [PATCH 10/11] Use safeInvoke to avoid type coercion --- packages/core/src/components/popover/popover.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index 1d9c8ef52c..ad29b63366 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -208,7 +208,7 @@ export class Popover extends AbstractPureComponent public componentDidMount() { this.updateDarkParent(); - this.popperObserver = new ResizeObserver(() => this.popperScheduleUpdate && this.popperScheduleUpdate()); + this.popperObserver = new ResizeObserver(() => Utils.safeInvoke(this.popperScheduleUpdate)); } public componentWillReceiveProps(nextProps: IPopoverProps) { From 294e2194a57f77b06de6a125fbec2374826a98b0 Mon Sep 17 00:00:00 2001 From: Byron Adams Date: Wed, 25 Jul 2018 10:39:17 +1200 Subject: [PATCH 11/11] follow convention when checking if ref exists --- packages/core/src/components/popover/popover.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index ad29b63366..32bd210a70 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -230,7 +230,7 @@ export class Popover extends AbstractPureComponent public componentDidUpdate() { this.updateDarkParent(); - if (this.popoverElement instanceof HTMLElement) { + if (this.popoverElement != null) { // Clear active observations to avoid the list growing. this.popperObserver.disconnect();