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: mixing bars with line or area series breaks legend toggle #410

Merged
merged 4 commits into from
Oct 9, 2019

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Oct 8, 2019

Summary

Fixes #399

PR410

The initial commit 4f2346e cast bandwidth in scales.ts in function computeXScale to 0 instead of the original issue where bandwidth was being divided by 0.

Commit 209560a changed bandwidth to be bandwidth/1 (instead of 0) which seems to be working as expected. My initial pass at this was to resolve the bug and get the x axis sizing appropriately based on the series that is visible. I plan to write or add to any unit tests but was hoping to get an initial first review to gather some feedback. Thanks!

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)
  • 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
  • Each commit follows the convention

@codecov-io
Copy link

codecov-io commented Oct 8, 2019

Codecov Report

Merging #410 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
+ Coverage      98%   98.12%   +0.12%     
==========================================
  Files          39       39              
  Lines        2853     3037     +184     
  Branches      681      748      +67     
==========================================
+ Hits         2796     2980     +184     
  Misses         50       50              
  Partials        7        7
Impacted Files Coverage Δ
src/chart_types/xy_chart/utils/scales.ts 100% <100%> (ø) ⬆️
...chart_types/xy_chart/utils/stacked_series_utils.ts 97.52% <0%> (-0.7%) ⬇️
src/chart_types/xy_chart/utils/series.ts 100% <0%> (ø) ⬆️
src/chart_types/xy_chart/domains/x_domain.ts 100% <0%> (ø) ⬆️
src/utils/commons.ts 94.73% <0%> (+0.29%) ⬆️
src/chart_types/xy_chart/rendering/rendering.ts 99.38% <0%> (+1.22%) ⬆️

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 81bac09...90ba912. Read the comment docs.

@rshen91
Copy link
Contributor Author

rshen91 commented Oct 8, 2019

I want to check on the computeXScale in 209560a to make sure the lines/bars/areas are adjusting correctly based on the other series not being there. The following numbers are taken from function getBandScaleRange in scales.ts:

No Bars (this used to be a problem) No Lines (this has been fine)
End 275.25 279
MaxRange 367 372
Bandwidth 91.75 93

In the Mixed - bars and lines story, the bars are wider than the line series, so it seems to be adjusting correctly to account for space when the wider series isn't part of the chart.

@elastic elastic deleted a comment Oct 8, 2019
@rshen91 rshen91 marked this pull request as ready for review October 8, 2019 20:14
@rshen91 rshen91 requested a review from nickofthyme October 8, 2019 20:14
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 (just a minor suggestion), since we are only hiding the bar series here, I think keeping a band type scale instead of switching to a continuous scale is the right approach in that situation and as shown in your gif it keeps the data representation the same, without rerendering the line only view to cover the full width.

It will be great if you can add a test for that, so we can catch any regression in the future

@@ -100,7 +99,7 @@ export function computeXScale(options: XScaleOptions): Scale {
range: [start, end],
},
{
bandwidth: bandwidth / totalBarsInCluster,
bandwidth: totalBarsInCluster > 0 ? bandwidth / totalBarsInCluster : bandwidth / 1,
Copy link
Member

@markov00 markov00 Oct 9, 2019

Choose a reason for hiding this comment

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

Suggested change
bandwidth: totalBarsInCluster > 0 ? bandwidth / totalBarsInCluster : bandwidth / 1,
bandwidth: totalBarsInCluster > 0 ? bandwidth / totalBarsInCluster : bandwidth,

@markov00 markov00 changed the title fix: Mixing bars with line or area series breaks legend toggle fix: mixing bars with line or area series breaks legend toggle Oct 9, 2019
@rshen91 rshen91 merged commit 57c0e3c into elastic:master Oct 9, 2019
@rshen91 rshen91 deleted the 399-bars branch October 9, 2019 16:17
markov00 pushed a commit that referenced this pull request Oct 9, 2019
## [13.5.1](v13.5.0...v13.5.1) (2019-10-09)

### Bug Fixes

* mixing bars with line or area series breaks legend toggle ([#410](#410)) ([57c0e3c](57c0e3c)), closes [#399](#399)
@markov00
Copy link
Member

markov00 commented Oct 9, 2019

🎉 This PR is included in version 13.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Oct 9, 2019
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
released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mixing bars with line or area series breaks legend toggle
4 participants