-
Notifications
You must be signed in to change notification settings - Fork 120
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: render stacked bar with stringified values #488
fix: render stacked bar with stringified values #488
Conversation
The stacked bars are computed adding up the previous value with the current one. This works fine if the passed values are numbers. If the number is codified as a string, the resulting stacked value is a wrongly concatenated string of values. This commit cast every y value to a number, if NaN or null it will use null. fix elastic#487
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Codecov Report
@@ Coverage Diff @@
## master #488 +/- ##
==========================================
+ Coverage 83.82% 83.85% +0.02%
==========================================
Files 158 169 +11
Lines 4724 4880 +156
Branches 961 981 +20
==========================================
+ Hits 3960 4092 +132
- Misses 749 772 +23
- Partials 15 16 +1
Continue to review full report at Codecov.
|
🎉 This PR is included in version 15.0.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [15.0.5](elastic/elastic-charts@v15.0.4...v15.0.5) (2019-12-12) ### Bug Fixes * render stacked bar with stringified values ([opensearch-project#488](elastic/elastic-charts#488)) ([24b9351](elastic/elastic-charts@24b9351)), closes [opensearch-project#487](elastic/elastic-charts#487)
Summary
In a stacked bar chart, the vertical position of each bar is computed starting from the sum of the vertical position of each previous bar. This works fine if, in the dataset, the y value is expressed as a number.
If the y value in the dataset is a string type, the chart is rendered in a weird way: the vertical position of each bar is wrongly computed as a concatenation of each y value. This PR cast every y value to a number, or to
null
if the y value is NaN ornull
.It's not a good practice having a stringified number as the y value, but this gracefully render the chart also in this situation.
fix #487
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] Any consumer-facing exports were added tosrc/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