Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Rerender popovers when content resizes #2718

Merged
merged 11 commits into from
Jul 24, 2018
Merged

[Core] Rerender popovers when content resizes #2718

Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2b4c3c8
Add ResizeObserver to force popper to rerender when content resizes
Jul 24, 2018
15ae92c
Rework observer/resize implmentation, as was only working on intial r…
Jul 24, 2018
6394932
fix typo in comment
Jul 24, 2018
94078b7
Fixing popperUpdater type definition
Jul 24, 2018
3788f2a
Align with code style guides
Jul 24, 2018
ca0de37
Clean up implementation as per review
Jul 24, 2018
fdfd8a3
Fixing lint error
Jul 24, 2018
6510801
Only initialise observer once, disconnect to clear observations
Jul 24, 2018
a9838d8
Reword comment
Jul 24, 2018
53d42af
Use safeInvoke to avoid type coercion
Jul 24, 2018
294e219
follow convention when checking if ref exists
Jul 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions packages/core/src/components/popover/popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -127,6 +128,12 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>
// element on the same page.
private lostFocusOnSamePage = true;

// 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 popperScheduleUpdate: () => void;

private refHandlers = {
popover: (ref: HTMLElement) => {
this.popoverElement = ref;
Expand Down Expand Up @@ -201,6 +208,7 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>

public componentDidMount() {
this.updateDarkParent();
this.updatePopperObserver();
}

public componentWillReceiveProps(nextProps: IPopoverProps) {
Expand All @@ -221,10 +229,18 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>

public componentDidUpdate() {
this.updateDarkParent();

if (this.popoverElement instanceof HTMLElement) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our convention for this check on refs is != null, no need to repeat the type.

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clear the observer before observing new elements. this will quickly result in a long list of watched elements, many of which may not be in the DOM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giladgray
Unfortunately the ResizeObserver spec doesn't allow removing observers if you don't have a reference to the target.

I've opted to just recreate the observer completely, does this seem reasonable you?

}
}

public componentWillUnmount() {
super.componentWillUnmount();
this.popperObserver.disconnect();
}

protected validateProps(props: IPopoverProps & { children?: React.ReactNode }) {
Expand Down Expand Up @@ -263,10 +279,21 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>
}
}

private updatePopperObserver() {
if (this.popperObserver instanceof ResizeObserver) {
this.popperObserver.disconnect();
}

this.popperObserver = new ResizeObserver(() => this.popperScheduleUpdate && this.popperScheduleUpdate());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to me that calling disconnect() should be enough, no need to recreate: https://wicg.github.io/ResizeObserver/#dom-resizeobserver-disconnect

  1. Clear the observationTargets list.

  2. Clear the activeTargets list.

please revert this so it's only created once and simply invoke disconnect before observing the new one.

}

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not the target node but the function instance itself!

this.popperScheduleUpdate = popperProps.scheduleUpdate;

const popoverHandlers: HTMLDivProps = {
// always check popover clicks for dismiss class
onClick: this.handlePopoverClick,
Expand Down