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.

Sign up for GitHub

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

Jump to bottom

fix(tooltip): placement with left/top legends and single bars #771

Merged
merged 9 commits into from
Aug 3, 2020
Merged

fix(tooltip): placement with left/top legends and single bars #771

merged 9 commits into from
Aug 3, 2020

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Jul 30, 2020 •
edited
Loading

Summary

Fixes #769 and #770

Related to #259

Left and Top Legend (#769)

Before

Screen Recording 2020-07-30 at 11 35 AM

After

Screen Recording 2020-07-30 at 11 25 AM

Single value bars (#770)

Reverses changes from #259 in favor of new popperjs changes that allow greater tooltip customization.

Before

Screen Recording 2020-07-30 at 12 03 PM

After

Screen Recording 2020-07-30 at 12 05 PM

Note: The ordinal tooltip uses a top placement because there is no room on left or right. This can be fixed by setting the tooltip.boundary prop. However, storybook renders the chart in an iframe that prevents us from fixing this.

Checklist

  • 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 nickofthyme added bug Something isn't working :tooltip Related to hover tooltip :xy Bar/Line/Area chart related labels Jul 30, 2020
@nickofthyme nickofthyme requested a review from markov00 July 30, 2020 16:40
@nickofthyme nickofthyme linked an issue Jul 30, 2020 that may be closed by this pull request
Bad tooltip placement for single bars #770
Closed
@nickofthyme nickofthyme changed the title fix(tooltip): placement with left and top legends fix(tooltip): placement with left and top legends and single bars Jul 30, 2020
@nickofthyme nickofthyme marked this pull request as ready for review July 30, 2020 18:07
@nickofthyme nickofthyme changed the title fix(tooltip): placement with left and top legends and single bars fix(tooltip): placement with left/top legends and single bars Jul 30, 2020
@markov00
Copy link
Member

Hey @nickofthyme why the tooltip behavior of a single bar histogram is different than the stacked bar one?
In this gif I've added multiple histograms stacked on each other and seems that this behavior is actually fine and better than pushing the tooltip outside the chart

Jul-31-2020 10-28-55

@markov00
Copy link
Member

We are going to integrate soon the small multiples, which means that the tooltip has to work over different areas independently from the legend position or its own panel. Is there a way to fix this without coupling this with the legend size? can we constraint the tooltip anchor to the chart panel only? Looking at the Before screenshots seems that the issue is related on the fact that we are not limiting the anchor inside the chart panel, do you think it's possible to achieve that without knowing the legend position/size?

markov00
markov00 approved these changes Aug 3, 2020
View reviewed changes
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.

Thanks for the legend size decouping, I've just one minor thing to ask about renaming:
can the globalChartDimension be renamed to globalOffset or only offset?
For everything else is good to merge after fixing the snapshot test

@nickofthyme nickofthyme merged commit e576b26 into elastic:master Aug 3, 2020
@nickofthyme nickofthyme deleted the fix-tooltip-placement branch August 3, 2020 15:25
markov00 pushed a commit that referenced this pull request Aug 10, 2020
@semantic-release-bot
chore(release): 21.0.0 [skip ci]
e7a8a30 
# [21.0.0](v20.0.2...v21.0.0) (2020-08-10)

### Bug Fixes

* update dep vulnerabilities, minimist and kind-of ([#763](#763)) ([4455281](4455281))
* **legend:** fix color anchor, add action context, fix action padding ([#774](#774)) ([4590a22](4590a22))
* **tooltip:** placement with left/top legends and single bars ([#771](#771)) ([e576b26](e576b26)), closes [#769](#769) [#770](#770)

### Features

* streamgraph and fit functions on stacked charts ([#751](#751)) ([268fcc0](268fcc0)), closes [#766](#766) [#715](#715) [#450](#450)

### BREAKING CHANGES

* the first parameter of `PointStyleAccessor` and `BarStyleAccessor` callbacks is changed from `RawDataSeriesDatum` to `DataSeriesDatum`. `stackAsPercentage` prop is replaced by `stackMode` that accept one `StackMode`.
@markov00
Copy link
Member

🎉 This PR is included in version 21.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Aug 10, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 21.0.0 [skip ci]
5c68eec 
# [21.0.0](elastic/elastic-charts@v20.0.2...v21.0.0) (2020-08-10)

### Bug Fixes

* update dep vulnerabilities, minimist and kind-of ([opensearch-project#763](elastic/elastic-charts#763)) ([843554f](elastic/elastic-charts@843554f))
* **legend:** fix color anchor, add action context, fix action padding ([opensearch-project#774](elastic/elastic-charts#774)) ([262f8d2](elastic/elastic-charts@262f8d2))
* **tooltip:** placement with left/top legends and single bars ([opensearch-project#771](elastic/elastic-charts#771)) ([75533b1](elastic/elastic-charts@75533b1)), closes [opensearch-project#769](elastic/elastic-charts#769) [opensearch-project#770](elastic/elastic-charts#770)

### Features

* streamgraph and fit functions on stacked charts ([opensearch-project#751](elastic/elastic-charts#751)) ([6f6a8cb](elastic/elastic-charts@6f6a8cb)), closes [opensearch-project#766](elastic/elastic-charts#766) [opensearch-project#715](elastic/elastic-charts#715) [opensearch-project#450](elastic/elastic-charts#450)

### BREAKING CHANGES

* the first parameter of `PointStyleAccessor` and `BarStyleAccessor` callbacks is changed from `RawDataSeriesDatum` to `DataSeriesDatum`. `stackAsPercentage` prop is replaced by `stackMode` that accept one `StackMode`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working released Issue released publicly :tooltip Related to hover tooltip :xy Bar/Line/Area chart related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bad tooltip placement for single bars Bad tooltip placement with left/top legend
2 participants
@nickofthyme @markov00