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

fix: tooltip container scroll issue #647

Merged
merged 9 commits into from
Apr 28, 2020

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Apr 23, 2020

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

Screen Recording 2020-04-23 at 11 15 AM

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.

Screen Recording 2020-04-27 at 08 58 AM

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.

Screen Recording 2020-04-27 at 09 03 AM

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.

Screen Recording 2020-04-23 at 09 17 AM

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
@nickofthyme nickofthyme added bug Something isn't working :tooltip Related to hover tooltip labels Apr 23, 2020
@nickofthyme nickofthyme requested review from markov00 and rshen91 April 23, 2020 16:55
Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

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

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.

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

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

Screenshot 2020-04-24 at 11 49 01

3) all charts are suffering from the same issue:

Screenshot 2020-04-24 at 11 49 16

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

@nickofthyme nickofthyme requested a review from markov00 April 27, 2020 14:05
@nickofthyme nickofthyme requested a review from rshen91 April 27, 2020 14:16
@nickofthyme
Copy link
Collaborator Author

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

Screen Recording 2020-04-27 at 05 30 PM

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            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.

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 tested locally and seems to work perfectly.
I've left a few minor comments, that are not required for merge

padding = 10,
): {
left: string | null;
top: string | null;
anchor: 'left' | 'right';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*
* @unit px
*/
static maxWidth = 256;
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
static maxWidth = 256;
static MAX_WIDTH = 256;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*
* @unit px
*/
static maxWidth = 256;
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
static maxWidth = 256;
static MAX_WIDTH = 256;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

/** the tooltip anchor computed position not adjusted within chart bounds */
anchorPosition: TooltipAnchorPosition,
): {
left: string | null;
top: string | null;
anchor: 'left' | 'right';
Copy link
Member

Choose a reason for hiding this comment

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

Position.Left | Position.Right

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 f411771 into elastic:master Apr 28, 2020
@nickofthyme nickofthyme deleted the fix/tooltip-scroll branch April 28, 2020 14:41
markov00 pushed a commit that referenced this pull request Apr 28, 2020
# [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
Copy link
Member

🎉 This PR is included in version 19.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Apr 28, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released Issue released publicly :tooltip Related to hover tooltip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Partition style chart (pie, treemap, etc) tooltips cause scrollbars to appear.
4 participants