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

feat(tooltip): improve positioning with popperjs #651

Merged
merged 52 commits into from
May 28, 2020

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Apr 24, 2020

Summary

This replaced our current tooltip positioning logic with popperjs.

Why bother? Well positioning is tricky and based on the current implementation there are some limitations to positioning relative to the window, this comes into play for really small charts.

View on local storybook: http://localhost:9001/?path=/story/bar-chart--test-tooltip-and-rotation

There is now a Portal component that is simply a presentation component that renders it's content to the root of the dom. You are able to pass the portal an anchor in which the portal will use to position itself. This anchor can be an HTMLElement or a postion values relative to a given ref HTMLElement.

These changes also allow greater flexibility in the size, specifically the width, of the tooltip. Currently the width is set manually for the tooltip to grow. However, with these changes, the tooltip can have a max-width and grow to that values without collapsing early. This also applies to any CustomTooltip component that is used in place of the default.

New Tooltip options

export interface TooltipProps {
  /**
   * Preferred placement of tooltip relative to anchor.
   *
   * This may not be the final placement given the positioning fallbacks.
   *
   * @default Placement.Right
   */
  placement?: Placement;
  /**
   * If given tooltip placement is not sutable, these `Placement`s will
   * be used as fallback placements.
   */
  fallbackPlacements?: Placement[];
  /**
   * Render custom tooltip given header and values
   */
  customTooltip?: CustomTooltip;
}
type Placement = "top" | "bottom" | "left" | "right" | "top-start" | "top-end" | "bottom-start" | "bottom-end" | "right-start" | "right-end" | "left-start" | "left-end" | "auto" | "auto-start" | "auto-end"

Changes

This works by creating a pseudo anchor element from the calculated AnchorPosition values.

Screen Recording 2020-04-29 at 02 40 PM

Notice the blue element behind the chart for illustration purposes.

Why Popperjs?

Pros:

  • Trusted source used for many applications
  • No need to tinker with countless positioning edge cases
  • Positions based on window or supplied boundary element. This would allow for optionally allowing the tooltip to go outside of the chart area (i.e. for smaller charts), or confine the tooltip to the chart element if desired.
  • It allows the option to set default placement and multiple fullbacks. Try right, else left, else top, else bottom.
  • More control over popover placement without fear of regressions

Cons:

Checklist

Delete any items that are not applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility, including a check against IE11
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

nickofthyme and others added 4 commits April 23, 2020 11:40
- refactor tooltip logic to account for portal
- seperate tooltip and tooltip portal components
- update tests to check for new portal condition
- set max tooltip width from component js
@nickofthyme nickofthyme added enhancement New feature or request :tooltip Related to hover tooltip labels Apr 24, 2020
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Good changes! I tested it on OS X with Chrome, Safari and FireFox, various chart types, various browser zoom levels via the menu, various pinch zoom levels and various scroll positions (by eg. shrinking the story page to lure out a vertical scrollbar).

323 files changed, many of them apparently unrelated to the goal of the PR, it'd be great to write up those changes so those can be isolated and reviewed too.

Also, a bunch of images changed; it looks initially OK as they seem to be interaction related, but not sure if the image changes are desired, eg. axes and even tooltip got removed here and there, eg.
image unless I'm looking at it the wrong way

@nickofthyme
Copy link
Collaborator Author

nickofthyme commented May 27, 2020

@monfera yeah I added an eslint rule for react hooks that required some file changes, namely the correct component naming of functional components. This required changing all the example story variables to Example, but that is all contained in a single commit.

The screenshots are off because I was changing the story to allow more uniform padding around the chart. I will add this to the other PR with the other changes to vrt. see 9c534e7

@monfera
Copy link
Contributor

monfera commented May 27, 2020

Thanks @nickofthyme - makes sense. I suppose it'll cause the tooltip(s) to return to those screengrabs too

@monfera
Copy link
Contributor

monfera commented May 27, 2020

Eg. here the tooltip seems to have moved away
image
and there's another very similar one.

There's also this where, unlike with most cases, the tooltip positioning is less good than before (top protrudes, is clipped), is it a problem?
image

@nickofthyme
Copy link
Collaborator Author

nickofthyme commented May 27, 2020

Yup. The first screenshot is now containing the tooltip beyond the chart container, which is an expected change. The placement is set to top and before would fallback to bottom to be contained within the chart element. We now allow placement relative to the window or a defined boundary such as the chart container. This was the root of the issue with #596 and the main motivation to use popper.

The second screenshot is also intended. The cursor is now centered relative to the tooltip. There is an option for right-start to position the tooltip how it currently is but I think this new placement is better.

@markov00 / @rshen91 any thoughts on this? see demo below. Notice the vertical placement of the tooltip relative to the cursor.

Current

Screen Recording 2020-05-27 at 12 53 PM

New

Screen Recording 2020-05-27 at 12 54 PM

@monfera
Copy link
Contributor

monfera commented May 27, 2020

Thanks @nickofthyme I played with it some more around the edge and it's very nice, there's no cropping of the tooltip so it's all good. That it looks cropped is simply due to where the test mouse position is and what area is screenshot

@monfera monfera self-requested a review May 27, 2020 18:48
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Looks great! A lot of attention to detail 🎉 Thanks for the updates. Good to merge, unless there's any unresolved ask from @markov00 or @rshen91

Comment on lines +681 to +683
Start: LineAlignSetting;
Center: LineAlignSetting;
End: LineAlignSetting;
Copy link
Member

Choose a reason for hiding this comment

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

weird....I will open a PR to fix this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, wasn't sure what this was from.

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM, I've pushed two small commits to fix some missing tsdocs and exported types


/**
* {@inheritDoc (Placement:variable)}
* @public
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@markov00 Is this required? I thought @public was the default.

Comment on lines +214 to 219
/**
* Unit for event (i.e. `time`, `feet`, `count`, etc.).
* Not currently used/implemented
* @alpha
*/
unit?: string;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@nickofthyme nickofthyme merged commit 6512950 into elastic:master May 28, 2020
@nickofthyme nickofthyme deleted the feat/popper-poc branch May 28, 2020 14:03
@nickofthyme nickofthyme changed the title feat: improve tooltip positioning with popperjs feat(tooltip): improve positioning with popperjs May 28, 2020
markov00 pushed a commit that referenced this pull request May 28, 2020
# [19.4.0](v19.3.0...v19.4.0) (2020-05-28)

### Bug Fixes

* **partition:** consider legendMaxDepth on legend size ([#654](#654)) ([9429dcf](9429dcf)), closes [#639](#639)

### Features

* **partition:** enable grooves in all group layers ([#666](#666)) ([f5b4767](f5b4767))
* **partition:** linked text overflow avoidance ([#670](#670)) ([b6e5911](b6e5911)), closes [#633](#633)
* **partition:** monotonic font size scaling ([#681](#681)) ([ea2489b](ea2489b)), closes [#661](#661)
* **tooltip:** improve positioning with popperjs ([#651](#651)) ([6512950](6512950)), closes [#596](#596)
@markov00
Copy link
Member

🎉 This PR is included in version 19.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label May 28, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released Issue released publicly :tooltip Related to hover tooltip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants