[Popover] Fix issue with Popover not being openable in a small Scrollable container

[JoelBesada]

WHY are these changes introduced?

(First Pull Request, here we go!)

Resolves #544.

It seemed like the implementation of intersectionWithViewport within the Popover component returned some (what I would assume to be) unintended values in certain cases. In the case reported in the issue above, a Rect with a negative height is returned. I changed the implementation so this shouldn't happen anymore.

I couldn't find any more context around this however, since the commit that added this function points to a PR that is not available here. I can only assume the expected behaviour based on the name of the function, but since I don't know the full intention of the original implementation I might be breaking something else here that's not caught by the tests. Please let me know!

WHAT is this pull request doing?

• Changes the intersectionWithViewport implementation to correctly clip the given rect
• Adds a couple of test cases for intersectionWithViewport

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

You should be able to open and close the popover within the modal in the Playground example.

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {Page, ActionList, Button, Popover, Modal} from '@shopify/polaris';
import {noop} from '@shopify/javascript-utilities/other';

interface State {
  active: boolean;
}

export default class Playground extends React.Component<never, State> {
  state = {
    active: true,
  };

  togglePopover = () => {
    this.setState(({active}) => {
      return {active: !active};
    });
  };

  render() {
    const activator = (
      <Button onClick={this.togglePopover}>More actions</Button>
    );

    return (
      <Page title="Playground">
        <Modal title="Testing" open onClose={noop}>
          <Modal.Section>
            <Popover
              active={this.state.active}
              activator={activator}
              onClose={this.togglePopover}
            >
              <ActionList items={[{content: 'Import'}, {content: 'Export'}]} />
            </Popover>
          </Modal.Section>
        </Modal>
      </Page>
    );
  }
}

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated UNRELEASED.md

[ghost]

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

[danrosenthal]

Thanks so much for the contribution @JoelBesada! We'll give this a look through and get back to you

Also, would you mind adding an entry to UNRELEASED.md for this change?

[tmlayton]

Could you add a test for what is expected if there is no intersection with the viewport?

[tmlayton]

And the 🎩 looks good, thanks for this fix!

[JoelBesada]

Thanks @tmlayton and @danrosenthal. I've added the test case for no intersection.

[AndrewMusgrave]

Re-pinged reviewers for a fresh set of 👀

[danrosenthal]

My 🎩 in the playground and of various components using Popover in the yarn tophat instance looked great. Code changes improve readability and expand test coverage. This is fantastic!

[danrosenthal]

This change makes the code so much more readable 👍

Feedback addressed

[AndrewMusgrave]

Looks like this is ready for review pinging reviewers again. cc/ @tmlayton @dleroux @solonaarmstrong

[danrosenthal]

@JoelBesada would you mind resolving this conflict so I can go ahead and merge?

(unless any pinged reviewers have objections)

[JoelBesada]

@danrosenthal resolved

[danrosenthal]

Thanks! I'll merge on 🍏

[ghost]

🎉 Thanks for your contribution to Polaris React!