[Core] Rerender popovers when content resizes #2718
Conversation
|
|
||
| if (this.popoverElement instanceof HTMLElement) { | ||
| // Ensure our observer has an up-to-date reference to popoverElement | ||
| this.popperObserver.observe(this.popoverElement); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
| @@ -201,6 +208,7 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState> | |||
|
|
|||
| public componentDidMount() { | |||
| this.updateDarkParent(); | |||
| this.popperObserver = new ResizeObserver(() => this.popperUpdater && this.popperUpdater()); | |||
There was a problem hiding this comment.
add this.popperObserver.disconnect() in componentWillUnmount
| private popperObserver: ResizeObserver; | ||
|
|
||
| // Reference to the Poppper.scheduleUpdate() function, this changes every time the popper is mounted | ||
| private popperUpdater: () => void; |
There was a problem hiding this comment.
popperScheduleUpdate? schedulePopperUpdate? make the name obvious.
| @@ -288,6 +301,8 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState> | |||
| this.props.popoverClassName, | |||
| ); | |||
|
|
|||
| this.popperUpdater = popperProps.scheduleUpdate; | |||
There was a problem hiding this comment.
do this as the first line in this method rather than breaking up this rendering logic, with a // comment about storing it on instance because it can change on every render.
|
Thanks for taking the time to review this @giladgray, have implemented your suggestions with the one exception, please take a look at my comments |
| this.popperObserver.disconnect(); | ||
| } | ||
|
|
||
| this.popperObserver = new ResizeObserver(() => this.popperScheduleUpdate && this.popperScheduleUpdate()); |
There was a problem hiding this comment.
seems to me that calling disconnect() should be enough, no need to recreate: https://wicg.github.io/ResizeObserver/#dom-resizeobserver-disconnect
Clear the observationTargets list.
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. |
There was a problem hiding this comment.
not the target node but the function instance itself!
| @@ -201,6 +208,7 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState> | |||
|
|
|||
| public componentDidMount() { | |||
| this.updateDarkParent(); | |||
| this.popperObserver = new ResizeObserver(() => this.popperScheduleUpdate && this.popperScheduleUpdate()); | |||
There was a problem hiding this comment.
finally, let's be typesafe and avoid coercion. use our safeInvoke util here.
new ResizeObserver(() => safeInvoke(this.popperScheduleUpdate))| @@ -221,10 +229,19 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState> | |||
|
|
|||
| public componentDidUpdate() { | |||
| this.updateDarkParent(); | |||
|
|
|||
| if (this.popoverElement instanceof HTMLElement) { | |||
There was a problem hiding this comment.
our convention for this check on refs is != null, no need to repeat the type.
|
Awesome! thanks @giladgray looking forward to 3.1! 👍 |
Fixes #2684 / fixes #692
Changes proposed in this pull request:
Proposed solution is to implement ResizeObserver as mentioned here to monitor for resizes on
this.popoverElement.I found the performance severely impacted if blindly creating an observer on every call to
Popover.renderPopover, so instead initialise the observer on mount and then update reference to thePopper.scheduleUpdateon every call toPopover.renderPopoverinstead.Realised that the DOM node the observer is watching may get removed when the popover is unmounted, so it would stop working, decided to update this on every
componentDidUpdatewhich seems to work much better.I am unsure how to implement tests for this change, but if they are required I can spend more time looking into this.
Screenshot
Before
After