-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat: fill multi-series with missing x values data points #409
Conversation
Codecov Report
@@ Coverage Diff @@
## master #409 +/- ##
==========================================
- Coverage 98% 97.95% -0.05%
==========================================
Files 39 39
Lines 2853 2887 +34
Branches 681 692 +11
==========================================
+ Hits 2796 2828 +32
- Misses 50 52 +2
Partials 7 7
Continue to review full report at Codecov.
|
When displaying a stacked area/line chart with multiple data sets we need to fill the dataset with zeros on missing data points. The rendering is affected also: it will hide the filled missing point but will continue to show the area beneath it. fix elastic#388
89c9c48
to
64a3c9e
Compare
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.
Tested locally, LGTM.
This raises a lot of possible edge cases but I think you captured them all. At least that I can think of right now.
@@ -20,6 +30,7 @@ export interface RawDataSeriesDatum { | |||
} | |||
|
|||
export interface DataSeriesDatum { | |||
/** the x value */ |
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.
😄
@@ -91,13 +104,13 @@ export function splitSeries( | |||
): { | |||
rawDataSeries: RawDataSeries[]; | |||
colorsValues: Map<string, any[]>; | |||
xValues: Set<any>; | |||
xValues: Set<string | number>; |
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.
nice I like the no any
!
for (let x of missingXValues.values()) { | ||
const stack = stackMap.get(x) || new Array(dataseries.length).fill(0); | ||
// currently filling as 0 value | ||
stack[index] = 0; |
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.
👍
} | ||
} | ||
newData.sort((a, b) => { | ||
if (xScaleType === ScaleType.Ordinal || typeof a.x === 'string' || typeof b.x === 'string') { |
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.
👍
return legendValues.map((value, index) => { | ||
const yAccessor: AccessorType = index === 0 ? AccessorType.Y1 : AccessorType.Y0; | ||
return ( | ||
<LegendItem |
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.
👍 I missed that it was in a div
🤦♂
🎉 This PR is included in version 13.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [13.5.0](elastic/elastic-charts@v13.4.1...v13.5.0) (2019-10-09) ### Features * **data:** fill datasets with zeros with missing points when stacked ([opensearch-project#409](elastic/elastic-charts#409)) ([b989074](elastic/elastic-charts@b989074)), closes [opensearch-project#388](elastic/elastic-charts#388)
Summary
fix #388
Before the fix
After the fix:
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.src/index.ts
(and stories only import from../src
except for test data & storybook)