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: position tooltip within chart with single value xScale #259

Merged

Conversation

emmacunningham
Copy link
Contributor

@emmacunningham emmacunningham commented Jul 3, 2019

Summary

fix #254

This PR introduces a fix for the tooltip positioning on a chart with a single value xScale.

I'm not entirely sure where the best place to position the tooltip, so this presents the minimal effort version, where the tooltip will be positioned inside of the chart aligned along the chart's edge (left if horizontal rotation, top if vertical rotation):

single_value

Implementation-wise, we've added a method isSingleValue to the Scale classes so that we can check if a scale is a single value scale (so that we can apply the different positioning in that case). The customDomain knob was added to the stories to demonstrate that, when the chart only has one datapoint but has a multiple value scale, the tooltips are positioned as usual (since there is room to display them as normal).

Checklist

Use strikethroughs to remove checklist items you don't feel are 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)
- [ ] Proper documentation or storybook story was added for features that require explanation or tutorials

@emmacunningham emmacunningham added wip work in progress :interactions Interactions related issue labels Jul 3, 2019
@emmacunningham emmacunningham changed the title Fix/single datum tooltip fix: position tooltip within chart with single value xScale Jul 3, 2019
@codecov-io
Copy link

codecov-io commented Jul 3, 2019

Codecov Report

Merging #259 into master will increase coverage by 0.41%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
+ Coverage   97.84%   98.26%   +0.41%     
==========================================
  Files          36       36              
  Lines        2693     3170     +477     
  Branches      607      774     +167     
==========================================
+ Hits         2635     3115     +480     
+ Misses         51       44       -7     
- Partials        7       11       +4
Impacted Files Coverage Δ
src/lib/utils/scales/scale_band.ts 94.11% <100%> (+0.36%) ⬆️
src/state/crosshair_utils.ts 91.66% <100%> (+6.37%) ⬆️
src/lib/utils/interactions.ts 100% <100%> (ø) ⬆️
src/lib/utils/scales/scale_continuous.ts 94.11% <100%> (+0.31%) ⬆️
src/state/chart_state.ts 97.44% <100%> (+0.54%) ⬆️
src/lib/series/stacked_series_utils.ts 96.47% <0%> (-1.75%) ⬇️
src/lib/series/specs.ts 100% <0%> (ø) ⬆️
src/lib/series/series.ts 100% <0%> (ø) ⬆️
src/lib/series/domains/y_domain.ts 100% <0%> (ø) ⬆️
src/lib/axes/canvas_text_bbox_calculator.ts 100% <0%> (ø) ⬆️
... and 6 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 af92736...121a8fb. Read the comment docs.

@emmacunningham emmacunningham marked this pull request as ready for review July 8, 2019 17:03
@emmacunningham emmacunningham removed the wip work in progress label Jul 8, 2019
@emmacunningham emmacunningham added the bug Something isn't working label Jul 8, 2019
@markov00
Copy link
Member

markov00 commented Jul 9, 2019

hey @emmacunningham just a suggestion, what if in the "single value" mode we switch between the "crosshair/vertical" mode and we manually enable the "follow" mode?

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Tested locally, LGTM. Just two comments.

stories/bar_chart.tsx Outdated Show resolved Hide resolved
stories/bar_chart.tsx Outdated Show resolved Hide resolved
@emmacunningham
Copy link
Contributor Author

hey @emmacunningham just a suggestion, what if in the "single value" mode we switch between the "crosshair/vertical" mode and we manually enable the "follow" mode?

@markov00 yes, good idea. have implemented that logic here: 121a8fb

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Changes look good

@emmacunningham emmacunningham merged commit f458bc9 into elastic:master Jul 15, 2019
markov00 pushed a commit that referenced this pull request Jul 15, 2019
## [8.0.1](v8.0.0...v8.0.1) (2019-07-15)

### Bug Fixes

* position tooltip within chart with single value xScale ([#259](#259)) ([f458bc9](f458bc9))
@markov00
Copy link
Member

🎉 This PR is included in version 8.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jul 15, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
## [8.0.1](elastic/elastic-charts@v8.0.0...v8.0.1) (2019-07-15)

### Bug Fixes

* position tooltip within chart with single value xScale ([opensearch-project#259](elastic/elastic-charts#259)) ([6c6c2d7](elastic/elastic-charts@6c6c2d7))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working :interactions Interactions related issue released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Single datum chart on element hover tooltip overflows the chart
4 participants