✨ [Popover] New mutationObserveConfig prop

[beefchimi]

This PR adds a new mutationObserveConfig?: MutationObserverInit; prop to the PositionedOverlay component.

Within Polaris, the PositionedOverlay component is used by both PopoverOverlay and TooltipOverlay. At the moment, I only have an interest in leveraging mutationObserveConfig for Popover. So, both Popover and PopoverOverlay have been updated to include the new mutationObserveConfig prop. I have left TooltipOverlay alone for now.

The motivation for this PR stems from a need to have dynamically updating content within a Popover for use within Online Store. We are implementing some new UI that will utilize a Collapsible within a Popover. As it stands currently, toggling a nested Collapsible will not always update the dimensions of the Popover. This is because the MutationObserver used within PositionedOverlay ignores everything except childList and subtree. By opening up the options argument of .observe() for customization, I am now able to pass in attributes: true as part of the options object, enabling a re-render of the Popover when Collapsible changes.

Props go to @AndrewMusgrave for helping me get to this solution.

Playground code

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

import {ActionList, Button, Collapsible, Page, Popover} from '../src';
import type {ActionListProps, ButtonProps, CollapsibleProps} 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'},
];

const requiredMutationConfig = {
  attributes: true,
};

export function Playground() {
  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),
    [],
  );

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

  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
        // Try commenting out this prop to see how this breaks our Collapsible + Popover UI
        mutationObserveConfig={requiredMutationConfig}
        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>
    </>
  );
}

thing.mp4

[github-actions]

size-limit report

Path	Size
cjs	142.55 KB (+0.02% 🔺)
esm	96.33 KB (+0.04% 🔺)
esnext	139.53 KB (+0.03% 🔺)
css	33.75 KB (0%)

[kyledurand]

Code looks good just waiting on the tests for PositionedOverlay. Was there some difficulty testing that component or just haven't gotten to it yet?

[kyledurand]

Suggested change

	- `Popover` and `PositionedOverlay` can now accept a `mutationObserveConfig` prop ([#4303](https://github.com/Shopify/polaris-react/pull/4303))
	- Added `mutationObserveConfig` prop to `Popover` and `PositionedOverlay` ([#4303](https://github.com/Shopify/polaris-react/pull/4303))

Per our guidelines

Past tense verb + brief issue/enhancement description in <ComponentName>

[beefchimi]

Anyone know of a good example I can reference for testing a mutation observer?

[kyledurand]

Off the top of my head you might be able to use window.dispatchEvent to trigger this but @AndrewMusgrave might have a more thorough idea on how to test this

[AndrewMusgrave]

I don't think we have any example of testing mutation observer. But we might be able to override the prototype & spy on it?

  // ⬇️  untested "pseudo" code
  let mutationObserverObserveSpy: jest.SpyInstance;

  beforeEach(() => {
    mutationObserverObserveSpy = jest.spyOn(MutationObserver.prototype, 'observe');
  });

  afterEach(() => {
    mutationObserverObserveSpy.mockRestore();
  });
  
  it(...) {
  ...
  expect(mutationObserverObserveSpy).toHaveBeenCalledWith(expect.any(Function), CONFIG)
  } 
  

[beefchimi]

Nice - thanks @AndrewMusgrave .

I needed to use the DOM node for the first argument, but other than that, seems like this worked as intended 👍

@kyledurand / @AndrewMusgrave mind giving another review?

Another thing to consider... and @clauderic can chime in on this as well, but perhaps we want listening for attributes changes to be the default behaviour? Maybe we don't even need this new mutationObserveConfig prop? What do you both think?

[clauderic]

I'd lean towards listening to attribute mutations by default and not opening this up to consumers.

Polaris components should ideally work well together out of the box without having to dive into complex configuration steps. You should be able to use a Collapsible component within a Popover without any additional config IMO.

If attributes change, it means that the children of the popover have changed for some reason, and should typically be an indication that the layout has changed (for example, if the class attribute changes or the style attribute). Of course there may also be attributes that change that have no impact on the layout.

Listening for attributes by default definitely means that there could potentially be more mutations reported and thus that the logic for measuring / positioning the popover may be called more often. In my opinion that's not a major performance concern so long as the resize / positioning logic is throttled or debounced.

[beefchimi]

@kyledurand / @AndrewMusgrave what are your thoughts on @clauderic comments? Should I go ahead with the PR as is, or should we always observe attribute changes?

Depending on the answer, would we still want to support the new mutationObserveConfig prop? It does allow for more flexibility, but just how valuable of a feature that is for our consumers I don't know.

[AndrewMusgrave]

I agree with @clauderic. Polaris components should work out of the box. If adding attributes to the config fixes a bug, you shouldn't have to opt into the fix 😄 I don't suspect we'll face a performance issue, but if we do. We can always evaluate making an API change then!

[beefchimi]

@kyledurand

just waiting on the tests for PositionedOverlay

I could not figure out how to properly test this part of the code, and couldn't track down any existing tests for this within Polaris. Any chance you have a good reference to point to?

[kyledurand]

I sort of agree with what @clauderic is saying about Polaris components working together by default but it's hard to anticipate everything our consumers will do by combining our components. Polaris doesn't really have anyone working on it at the moment except for the teams that are using it. Adding this logic internally also adds complexity to the already overly complex Popover component. I'm down for you to 🚢 @beefchimi

[beefchimi]

Closing in favour of: #4372