⚗️ [Popover] Expose imperative relayout function

[beefchimi]

This PR continues to try and find a solution to my struggles with getting Popover to re-render its size dimensions while toggling nested Collapsible components.

There have been two previous attempts:

1. ✨ [Popover] New mutationObserveConfig prop #4303
2. ✨ [Popover] Now observes overlay attributes #4372

The final attempt has ended in numerous Polaris tests (which use Popover somewhere under the hood) stalling in what appears to be an endless re-render cycle. Something about the attributes MutationObserver is causing certain tests to never complete.

Some additional conversation can be found here:
https://shopify.slack.com/archives/C4Y8N30KD/p1628608394090200

Before I start fighting through each of these tests to attempt a resolution, I want us to first entertain this alternative approach.

Solution

This was first suggested by @clauderic - which is to offer an imperative API which enables consumer's to dictate exactly when they might need to re-render the Overlay dimensions.

I have wrapped Popover with forwardRef and exposed a forceReLayout() function via useImperativeHandle. Consumer's can retrieve the Popover instance by way of ref, and call popoverRef.current.forceReLayout(); whenever they need to update the dimensions.

This actually calls through multiple component layers to get at the code which handles updating measurements:
Popover > PopoverOverlay > PositionedOverlay.

It certainly is not the most elegant solution, but it hopefully leads us to a more explicit result. Rather than forcing the MutationObserver to watch for additional attributes and potentially incur some ramification, we can put control in the hands of consumer's and request a layout update on command.

Reviewers

I very much appreciate any review / advice reviewers can offer, as we are in desperate need of this functionality in Online Store. Some questions to consider:

• What do you think of this approach?
• What can I do to improve the solution?
• Are there pros / cons compared to the previous PR attempts?

Please tophat this PR using the following snippet:

Playground snippet:

import React, {useCallback, useRef, useState} from 'react';

import {ActionList, Button, Collapsible, Page, Popover} from '../src';
import type {
  ActionListProps,
  ButtonProps,
  CollapsibleProps,
  PopoverPublicAPI,
} from '../src';

interface CollapsibleSectionProps {
  id: string;
  title: string;
  items: ActionListProps['items'];
  open: CollapsibleProps['open'];
  onAction: ButtonProps['onClick'];
}

const POLARIS_COLLAPSIBLE_DISABLE_TRANSITION = {
  timingFunction: 'unset',
  duration: '1ms',
};

const popoverWrapperStyles = {
  width: 300,
  maxHeight: '50vh',
};

const pageItems: CollapsibleSectionProps['items'] = [
  {content: 'Import'},
  {content: 'Export'},
  {content: 'Something else'},
  {content: 'Shorter content'},
  {content: 'Spring loaded'},
  {content: 'Explore'},
  {content: 'Save'},
  {content: 'Delete'},
  {content: 'Very long thing'},
  {content: 'Final'},
];

const productItems: CollapsibleSectionProps['items'] = [
  {content: 'Another type of something else'},
  {content: 'Lots of stuff that gets written within this element'},
  {
    content:
      'It looks like when I use the fluidContent prop, I need to specify both a max-width and a max-height',
  },
  {
    content:
      'Otherwise, I will end up with a page full of Popover once it opens.',
  },
  {content: 'We need to provide sensible limits to our max Popover size'},
  {content: 'Final'},
];

export function Playground() {
  const popoverRef = useRef<PopoverPublicAPI>(null);

  function layoutOverlay() {
    // Wait a single frame to help defer until
    // after the `1ms` <Collapsible /> transition.
    requestAnimationFrame(() => popoverRef.current?.forceUpdatePosition());
  }

  const [popoverActive, setPopoverActive] = useState(false);
  const popoverActiveToggle = useCallback(
    () => setPopoverActive((popoverActive) => !popoverActive),
    [],
  );
  const popoverActiveClose = () => setPopoverActive(false);

  const popoverActivator = (
    <Button onClick={popoverActiveToggle} disclosure>
      More actions
    </Button>
  );

  const [openPages, setOpenPages] = useState(true);
  const toggleOpenPages = useCallback(() => {
    setOpenPages((openPages) => !openPages);
    layoutOverlay();
  }, []);

  const [openProducts, setOpenProducts] = useState(false);
  const toggleOpenProducts = useCallback(() => {
    setOpenProducts((openProducts) => !openProducts);
    layoutOverlay();
  }, []);

  return (
    <Page
      title="Prototype for Popover + Collapsible"
      subtitle="This code explores how we can use the Popover component to render dynamic content"
      primaryAction={{content: 'Primary action', disabled: true}}
    >
      <Popover
        ref={popoverRef}
        active={popoverActive}
        activator={popoverActivator}
        onClose={popoverActiveClose}
      >
        <div style={popoverWrapperStyles}>
          <CollapsibleSection
            id="PagesCollapsible"
            title="Pages"
            items={pageItems}
            open={openPages}
            onAction={toggleOpenPages}
          />

          <CollapsibleSection
            id="ProductsCollapsible"
            title="Products"
            items={productItems}
            open={openProducts}
            onAction={toggleOpenProducts}
          />
        </div>
      </Popover>
    </Page>
  );
}

function CollapsibleSection({
  id,
  title,
  items,
  open,
  onAction,
}: CollapsibleSectionProps) {
  return (
    <>
      <Button
        primary
        fullWidth
        textAlign="left"
        disclosure={open ? 'up' : 'down'}
        ariaControls={id}
        ariaExpanded={open}
        onClick={onAction}
      >
        {title}
      </Button>
      <Collapsible
        id={id}
        open={open}
        transition={POLARIS_COLLAPSIBLE_DISABLE_TRANSITION}
      >
        <ActionList items={items} />
      </Collapsible>
    </>
  );
}

[beefchimi]

This appears to be a difficult type to wrangle... and there major issues with me setting these to optional (?), or is there another way I can type this to keep these subcomponents as required?

[chloerice]

Hey Curtis 👋🏽 I played around with this and ended up 🔥 the PopoverSubcomponents interface and trying the second answer to this StackOverflow question and it works locally.

const PopoverNamespace = Object.assign(Popover, {Pane, Section});

export {PopoverNamespace as Popover};

I'm having trouble running web right now so I haven't been able to test on a consuming app (the comment on line 83 I'm guessing means this was a problem in the past 🤔 )

[clauderic]

These shouldn't be made optional. I'd go with something along the lines of what @chloerice suggested (assigning Pane and Section to Popover after you've stored a reference to the ref forwarded component in a separate variable):

export const Popover = Object.assign(PopoverComponentWithForwardedRef, {Pane, Section});

[clauderic]

Looks good overall. There's a significant amount of ref drilling going on, but there aren't other good solutions around it based on how these components are structured so LGTM.

[clauderic]

naming nitpick:

Suggested change

	forceReLayout(): void;
	forceUpdatePosition(): void;

[beefchimi]

The only point of push back I'd offer is that this also handles dimensions - so it is more than just "position". However, I'm okay with the name change, as under the hood it is calling handleMeasurement and that itself isn't the most explicit name.

[clauderic]

How about something like:

Suggested change

	export interface PopoverHandles {
	export interface PopoverPublicAPI {

[beefchimi]

*ComponentName*Handles actually seems to be a Polaris convention - having looked at other components that use forwardRef. However, I agree that PublicAPI is more explicit. It better communicates this types' responsibility (although you could argue that Props also belong within a "public API").

I'll go ahead and make the change, and if anyone from Polaris would like me to revert, I am okay with that.

[clauderic]

Suggested change

	export const Popover: PopoverComponentType = forwardRef(function Popover(
	const PopoverComponent = forwardRef<PopoverPublicAPI, PopoverPops>((props, ref) => {

[clauderic]

nit:

Suggested change

	requestAnimationFrame(() => this.handleMeasurement());
	requestAnimationFrame(this.handleMeasurement);

[clauderic]

These shouldn't be made optional. I'd go with something along the lines of what @chloerice suggested (assigning Pane and Section to Popover after you've stored a reference to the ref forwarded component in a separate variable):

export const Popover = Object.assign(PopoverComponentWithForwardedRef, {Pane, Section});

[github-actions]

size-limit report

Path	Size
cjs	142.64 KB (+0.08% 🔺)
esm	96.38 KB (+0.1% 🔺)
esnext	139.58 KB (+0.07% 🔺)
css	33.76 KB (0%)

[beefchimi]

@clauderic + @chloerice thanks so much for the review!

I've addressed all of your feedback and I think this should be ready for final review 😄

[beefchimi]

For anyone wondering - I've barely changes this file, but since we now wrap with a forwardRef, the indentation/formatting got updated and therefor its hard to really parse exact lines of code that have changed 😭

[beefchimi]

The changelog check is stuck for some reason, and I don't know how to re-trigger it. I'm going to try applying the skip changelog label to see if I can get unblocked.