Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new(Tooltip): Add popover prop #382

Merged
merged 4 commits into from
Jun 30, 2020
Merged

new(Tooltip): Add popover prop #382

merged 4 commits into from
Jun 30, 2020

Conversation

espressoroaster
Copy link

to: @williaster @hayes @alecklandgraf

Description

A popover is essentially a tooltip component with the ability to persist the popup by mouse-entering the popup portion of the tooltip.

This functionality is achieved by adding a delay timer and setTimeout (via delaySetPopupVisible) to ensure that the popup persists long enough for a mouse to move from tooltip trigger to popup div when the popover prop is selected. When a mouse enters the popup div, the timer is cleared, and the popup can persist until the mouse leaves the popup.

Motivation and Context

Several internal tools at Airbnb now require the ability to click links inside a popup, and copy-and-paste text in the popup. However, without this prop, it is not possible to interact with the contents of a popup in the Lunar tooltip

(remainOnMouseDown & toggleOnClick) props can be used to persist the popups, but we won't be able to interact with the info in the popup since renderPopup previously only rendered everything in an Overlay. Additionally, we can't use these props the way we'd like when wrapping a link component with a Tooltip that has these props -- you'd click the link when triggering these props. The timeout method via delaySetPopupVisible solves this problem.

Testing

Added Jest tests to cover the expected open and closing behavior of a popover. Tested locally in storybook.

Screenshots

image

image

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
Copy link
Collaborator

airbnb-bot commented Jun 25, 2020

Size Changes

Package Diff ESM Prev ESM CJS Prev CJS
core +0.3% 566.23 KB 564.56 KB 708.19 KB 706.5 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": 578112,
    "lib": 723452
  },
  "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": 579823,
    "lib": 725188
  },
  "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
  }
}

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactor! I had a couple of thoughts including updating Overlay so that we could use it here.

packages/core/src/components/Tooltip/index.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Tooltip/index.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Tooltip/index.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Tooltip/index.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Tooltip/index.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Tooltip/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left another round of comments. I pulled the branch locally and think we can probably opt out of using Overlay 👍 I think the UX is great!

I think if we can address these small thoughts we can get this puppy in 🐶 🚢

packages/core/src/components/Tooltip/index.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Tooltip/index.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Tooltip/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚢

Approved the happo diffs, they all look unrelated and seem to be chrome's rendering of emojis + icons (so likely a font change in chrome)

@@ -34,6 +36,8 @@ export type TooltipProps = {
onClose?: () => void;
/** Callback fired when the tooltip is shown. */
onShow?: () => void;
/** True to enable interactive popover functionality */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit but all of these should be full sentences / end in . 😬

@espressoroaster espressoroaster merged commit 19f4364 into master Jun 30, 2020
@espressoroaster espressoroaster deleted the fo--popover branch June 30, 2020 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants