update(Tooltip): Render Popover's popup in Overlay component

[espressoroaster]

to: @williaster @alecklandgraf

Description

This PR refactors Popover to render inside an <Overlay> component, just like the plain tooltip. It also moves the popup mouseEnter and mouseLeave handlers up to the popup content's outermost div.

Note: We'll want to land #391 first, and then update the target of this PR to master. I set the base of this PR to the Overlay PR to facilitate code review.

Motivation and Context

The original Popover component rendered in a simple div with a style of zIndex:1. This was naive on my part for several reasons:

1. The Popover would need a much larger zIndex to escape divs with zIndexes potentially much larger than 1.
2. In our Popover-inside-DataTable use case, the Popover was unable to escape a DataTable row, while a regular Tooltip, which rendered in Overlay's portal, was able to.

Testing

Tested with existing unit tests and new story book story.

Screenshots

Checklist

• My code follows the style guide of this project.
• I have updated or added documentation accordingly.
• I have read the CONTRIBUTING document.

[airbnb-bot]

Size Changes

Package	Diff	ESM	Prev ESM	CJS	Prev CJS
core	+0.0%	566.78 KB	566.56 KB	708.84 KB	708.61 KB

Compared to master. File sizes are unminified and ungzipped.

View raw build stats

Previous (master)

{
  "apollo": {
    "esm": 10832,
    "lib": 14147
  },
  "app-shell": {
    "esm": 12906,
    "lib": 19874
  },
  "composer": {
    "esm": 68247,
    "lib": 101805
  },
  "core": {
    "esm": 580157,
    "lib": 725612
  },
  "forms": {
    "esm": 37350,
    "lib": 49298
  },
  "icons": {
    "esm": 156355,
    "lib": 205626
  },
  "layouts": {
    "esm": 15298,
    "lib": 20770
  },
  "metrics": {
    "esm": 5467,
    "lib": 7729
  },
  "test-utils": {
    "esm": 4279,
    "lib": 5937
  }
}

Current

{
  "apollo": {
    "esm": 10832,
    "lib": 14147
  },
  "app-shell": {
    "esm": 12906,
    "lib": 19874
  },
  "composer": {
    "esm": 68247,
    "lib": 101805
  },
  "core": {
    "esm": 580380,
    "lib": 725857
  },
  "forms": {
    "esm": 37350,
    "lib": 49298
  },
  "icons": {
    "esm": 156355,
    "lib": 205626
  },
  "layouts": {
    "esm": 15298,
    "lib": 20770
  },
  "metrics": {
    "esm": 5467,
    "lib": 7729
  },
  "test-utils": {
    "esm": 4279,
    "lib": 5937
  }
}

[williaster]

Awesome! Had one question but LGTM overall.

Glad John will forever be immortalized in the new story 😅

[williaster]

can you elaborate on why this change was needed? Seems fine, wanted to check that the margin areas didn't trigger the event listeners tho (like to the top/left of the actual content)

[espressoroaster]

wanted to check that the margin areas didn't trigger the event listeners tho (like to the top/left of the actual content)

I can assure this is ok with a gif (hacked in a larger margin so we can see the gif easily).

can you elaborate on why this change was needed?

Yesterday Erik and I ran into a weird case where if the mouse was on the trigger while the popup was opening in the direction the mouse came from, there wouldn't be a continuity between mouse enter trigger -> mouse leave trigger (because of the popup animation) -> mouse entering popup -> mouse leaving popup (when animations passes the cursor) -> mouse back on trigger.

Oddly enough, I don't seem to be encountering that today if I move the mouse handlers back down to the content div. But the first screenshot shows margins weren't causing an issue with the current implementation, so I think the handlers should be fine on either div.

[williaster]

thanks for adding these 😄

[espressoroaster]

np, I said I would in the last PR :)