fix: tooltip container scroll issue by nickofthyme · Pull Request #647 · elastic/elastic-charts

nickofthyme · 2020-04-23T16:55:18Z

Summary

Fixes tooltip render issue

refactor tooltip logic to account for portal
separate tooltip and tooltip portal components
update tests to check for new portal condition
set max tooltip width from component js

fixes #596

Temporary fix

For really narrow charts where the tooltip cannot fit on the right nor the left, a temporary fix was added to limit the width of the tooltip portal to a max of 70% of the chart elements width to limit hiding the tooltip on the left. This is temporary until a more robust positioning logic is add in #651.

One issue with this approach is that the tooltip will go out of the element on the right for values in the center of the chart. However, this will not cause the scrollbars to show as described in #596.

This issue can also be seen with relatively normal widths where the tooltip portal does not fit in the space but the visible tooltip does, creating an unnecessary flip to the left side, which might cause the tooltip to appear outside of the element.

Root cause

Tooltips rendered towards the right edge of the chart container would compute if the tooltip "fits" based on the .echTooltip element and not the #echTooltipContainerPortal element. Notice the red bar rendered behind the tooltip as the #echTooltipContainerPortal element. Thus, when the .echTooltip element width is less than that of the #echTooltipContainerPortal the element will still render on the right but the #echTooltipContainerPortal created an invisible additional width that pushed the tooltip off the page.

The fix is to just account for the #echTooltipContainerPortal element width in the "fitting" calculation but still use the .echTooltip element width for final positioning.

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
Unit tests were updated or added to match the most common scenarios

- 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

rshen91

This LGTM! Tested locally in Chrome - no scrollbar in the playground nor in the stories when squishing the browser.

markov00

I've checked in our storybook and also on the PR playground and it doesn't seem to solve the problems once for all.

on the playground, (treemap example) if you get rid of the legend the scroll bar will appear
the same happens on the storybook for treemap, see:

3) all charts are suffering from the same issue:

I've open a partial PR with a fix for horizontal charts: nickofthyme#1

nickofthyme · 2020-04-27T22:36:13Z

Fixed annotation tooltips with the same fix as the regular tooltip.

codecov-io · 2020-04-27T22:49:56Z

Codecov Report

Merging #647 into master will increase coverage by 0.28%.
The diff coverage is 59.13%.

@@            Coverage Diff             @@
##           master     #647      +/-   ##
==========================================
+ Coverage   72.54%   72.82%   +0.28%     
==========================================
  Files         245      261      +16     
  Lines        8158     8446     +288     
  Branches     1597     1645      +48     
==========================================
+ Hits         5918     6151     +233     
- Misses       2207     2258      +51     
- Partials       33       37       +4

Impacted Files	Coverage Δ
...t_types/xy_chart/annotations/annotation_tooltip.ts	`7.14% <0.00%> (-1.20%)`	⬇️
src/chart_types/xy_chart/legend/legend.ts	`85.00% <ø> (ø)`
...ypes/xy_chart/renderer/dom/annotation_tooltips.tsx	`51.72% <18.18%> (-3.52%)`	⬇️
src/components/tooltip/tooltip.tsx	`26.66% <26.66%> (ø)`
src/state/reducers/interactions.ts	`70.96% <45.45%> (ø)`
src/components/tooltip/tooltip_portal.tsx	`76.66% <46.15%> (ø)`
src/components/legend/legend_item.tsx	`94.36% <96.00%> (ø)`
src/components/legend/color.tsx	`100.00% <100.00%> (ø)`
src/components/legend/label.tsx	`100.00% <100.00%> (ø)`
src/components/tooltip/index.ts	`100.00% <100.00%> (ø)`
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed91744...96784a3. Read the comment docs.

markov00

LGTM tested locally and seems to work perfectly.
I've left a few minor comments, that are not required for merge

markov00 · 2020-04-28T12:53:29Z

src/chart_types/xy_chart/annotations/annotation_tooltip.ts

 ): {
  left: string | null;
  top: string | null;
+  anchor: 'left' | 'right';


nit: we can use Position.Left and Position.Right here

markov00 · 2020-04-28T12:55:19Z

src/chart_types/xy_chart/renderer/dom/annotation_tooltips.tsx

+   *
+   * @unit px
+   */
+  static maxWidth = 256;


nit

Suggested change

static maxWidth = 256;

static MAX_WIDTH = 256;

markov00 · 2020-04-28T12:56:41Z

src/components/tooltip/tooltip_portal.tsx

+   *
+   * @unit px
+   */
+  static maxWidth = 256;


nit:

Suggested change

static maxWidth = 256;

static MAX_WIDTH = 256;

markov00 · 2020-04-28T12:57:08Z

src/components/tooltip/utils.ts

 ): {
  left: string | null;
  top: string | null;
+  anchor: 'left' | 'right';


Position.Left | Position.Right

# [19.0.0](v18.4.2...v19.0.0) (2020-04-28) ### Bug Fixes * tooltip container scroll issue ([#647](#647)) ([f411771](f411771)) * **annotations:** fix alignment at the edges ([#641](#641)) ([43c5a59](43c5a59)), closes [#586](#586) ### Features * shift click legend items & partition legend hover ([#648](#648)) ([ed91744](ed91744)) * **brush:** add multi axis brushing ([#625](#625)) ([9e49534](9e49534)), closes [#587](#587) [#620](#620) ### BREAKING CHANGES * **brush:** The type used by the `BrushEndListener` is now in the following form `{ x?: [number, number]; y?: Array<{ groupId: GroupId; values: [number, number]; }> }` where `x` contains an array of `[min, max]` values, and the `y` property is an optional array of objects, containing the `GroupId` and the values of the brush for that specific axis. * **annotations:** In the rectangular annotation, the y0 parameter of the coordinates now refers to the minimum value and the y1 value refers to the maximum value of the y domain.

markov00 · 2020-04-28T21:57:08Z

🎉 This PR is included in version 19.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [19.0.0](elastic/elastic-charts@v18.4.2...v19.0.0) (2020-04-28) ### Bug Fixes * tooltip container scroll issue ([opensearch-project#647](elastic/elastic-charts#647)) ([f0af34b](elastic/elastic-charts@f0af34b)) * **annotations:** fix alignment at the edges ([opensearch-project#641](elastic/elastic-charts#641)) ([c698d08](elastic/elastic-charts@c698d08)), closes [opensearch-project#586](elastic/elastic-charts#586) ### Features * shift click legend items & partition legend hover ([opensearch-project#648](elastic/elastic-charts#648)) ([cf15ca1](elastic/elastic-charts@cf15ca1)) * **brush:** add multi axis brushing ([opensearch-project#625](elastic/elastic-charts#625)) ([382cb14](elastic/elastic-charts@382cb14)), closes [opensearch-project#587](elastic/elastic-charts#587) [opensearch-project#620](elastic/elastic-charts#620) ### BREAKING CHANGES * **brush:** The type used by the `BrushEndListener` is now in the following form `{ x?: [number, number]; y?: Array<{ groupId: GroupId; values: [number, number]; }> }` where `x` contains an array of `[min, max]` values, and the `y` property is an optional array of objects, containing the `GroupId` and the values of the brush for that specific axis. * **annotations:** In the rectangular annotation, the y0 parameter of the coordinates now refers to the minimum value and the y1 value refers to the maximum value of the y domain.

fix: tooltip container scroll issue

5ee6ad0

- 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 added bug Something isn't working :tooltip Related to hover tooltip labels Apr 23, 2020

nickofthyme requested review from markov00 and rshen91 April 23, 2020 16:55

nickofthyme added 2 commits April 23, 2020 11:58

Merge branch 'master' into fix/tooltip-scroll

8a18b7a

fix: type error in dist

072b09f

rshen91 approved these changes Apr 23, 2020

View reviewed changes

markov00 requested changes Apr 24, 2020

View reviewed changes

markov00 and others added 2 commits April 24, 2020 09:10

fix: possibly fix the scrollbar in all charts

0140bd2

fix: set max width to 70% of container width

80839cf

nickofthyme requested a review from markov00 April 27, 2020 14:05

nickofthyme added 2 commits April 27, 2020 09:08

test: fix positioning tests

d2eb346

Merge branch 'master' into fix/tooltip-scroll

f159273

nickofthyme requested a review from rshen91 April 27, 2020 14:16

fix: annotations tooltips positioning

96784a3

markov00 approved these changes Apr 28, 2020

View reviewed changes

fix: variable naming

90fc234

nickofthyme merged commit f411771 into elastic:master Apr 28, 2020

nickofthyme deleted the fix/tooltip-scroll branch April 28, 2020 14:41

markov00 added the released Issue released publicly label Apr 28, 2020

markov00 mentioned this pull request Apr 29, 2020

Update dependency @elastic/charts to v19.1.2 elastic/kibana#64759

Merged

Conversation

nickofthyme commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Temporary fix

Root cause

Checklist

Uh oh!

rshen91 left a comment

Choose a reason for hiding this comment

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

nickofthyme commented Apr 27, 2020

Uh oh!

codecov-io commented Apr 27, 2020

Codecov Report

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markov00 commented Apr 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nickofthyme commented Apr 23, 2020 •

edited

Loading