feat: add band area chart #157

markov00 · 2019-04-08T14:19:17Z

Summary

close #144

This PR adds a y0Accessors prop for each SeriesSpec: this property can be used to describe a dataset with bands ( with a min and max value for each data point).
It's mainly used for with AreaSeries to descibe a bounded area chart, bounded on the y axis. The y0 value, is usually a lower value than the y1, it will be rendered perpendicularly to the y1 point. On a standard area chart, the y0 value usually correspond to 0 or to the previous value in the stack. In a band-area chart, the y0 can be any value below the y1 value.

It can also be applied to bar charts, with some limitations as described below. On a barchart, the y0 value is the bottom edge of the bar.

On a stacked bar/area chart a series with a y0Accessor will be stacked on top of the below series stacking the below series y1 value with the band series y0 value. This means that if the below series at a point X has an height of 10, and the bound series has the following values: y0: 2, y1: 5 the resulting chart will push the bound series to the top starting using the following values: y0:12, y1: 15

The IndexedGeometry object is also refactored. To limit amount of redundant code/memory object we used the same Geometry elements used on the highlight/hover process.
The structure of IndexedGeometry is slightly changed, to avoid duplicate values and to simplify tooltip formatting when rendering either y1 and y0 values.

A candlestick-like barchart can be also configured, but the y0 value is not currently displayed on the tooltip.

After few investigation on tests, I've currently removed from the indexedGeometries map all geometry with null values on y1. This leads to a minor problem: we are not showing any tooltip on that interval, the tooltip of the crosshair is never shown.

To Dos:

~~[ ] bring back animation on area charts~~ see Disable animation #161
add test on tooltip with stacked bars, check the right value for each bar
~~[ ] enable if possible the visualization of the y0 value for bars~~ see Display y0 and y1 names on tooltip for band charts #162

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

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 · 2019-04-10T12:38:26Z

Codecov Report

Merging #157 into master will increase coverage by 0.59%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
+ Coverage   95.66%   96.26%   +0.59%     
==========================================
  Files          34       36       +2     
  Lines        1801     1848      +47     
  Branches      228      239      +11     
==========================================
+ Hits         1723     1779      +56     
+ Misses         65       60       -5     
+ Partials       13        9       -4

Impacted Files	Coverage Δ
src/state/utils.ts	`91.22% <ø> (ø)`	⬆️
...onents/react_canvas/utils/rendering_props_utils.ts	`100% <ø> (ø)`	⬆️
src/lib/series/specs.ts	`100% <ø> (ø)`	⬆️
src/lib/series/rendering.ts	`89.58% <100%> (+2.91%)`	⬆️
src/lib/series/tooltip.ts	`100% <100%> (+24.13%)`	⬆️
src/lib/series/domains/y_domain.ts	`100% <100%> (ø)`	⬆️
src/lib/utils/interactions.ts	`100% <100%> (ø)`	⬆️
src/lib/series/nonstacked_series_utils.ts	`100% <100%> (ø)`
src/state/chart_state.ts	`96.21% <87.5%> (ø)`	⬆️
src/lib/series/stacked_series_utils.ts	`94.73% <94.73%> (ø)`
... and 4 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 21316d1...c2156d9. Read the comment docs.

Add a `y0Accessor` prop for each series to enable the possibility to add a lower limit to area and bars. close elastic#144

refactoring of the indexed geometries: removed the extra duplicated values in favour of a better rendering geometry.

This commit also remove the wrongly conceived seriesKey with an undefined element in there. It's now a clean empty array if you wrongly specified a wrong split accessor

The logic of data structure in stacked charts is changed a bit to reflect correctly what are the initial values of the datum and what are the current stacked data

emmacunningham

one tiny comment but overall code lgtm & tested locally on firefox. there are some places where we might be able to refactor to help with test coverage also, but since we are going to take care of this in a separate PR already anyways, i think that can be handled there.

a couple of things that can be addressed in separate PRs but just mentioning them here:

when we go back to improve the test coverage, maybe we can also add data with only negative values & mixed negative/positive. i checked the stories with data like this and the bands look fine still but might still be nice to have those in the tests to check in the future.

the other thing is that right now only one of the band values appears next to the legend element as you're hovering, which is especially noticeable when you're hovering over the top part of the band and the value next to the legend element is the value is for the bottom part of the band:

emmacunningham · 2019-04-10T22:53:34Z

src/lib/series/series.ts

+  y1: number | null;
+  /** the optional y0 metric, used for bars or area with a lower bound */
+  y0?: number | null;
+  /** the datum */
  datum?: any;
 }

 export interface DataSeriesDatum {


Could DataSeriesDatum extend RawDataSeriesDatum? Seems like there is a lot of overlap and since they are related, might make it clearer for others in the codebase to draw this connection.

They are similar but they have only few things in common. The raw, is more or less the initialY part of the DataSeriesDatum. I can check and rewrite a bit those

markov00 · 2019-04-11T14:47:01Z

@emmacunningham thanks for the review. I will address those problems in 3 separated PRs: one for testing negative/mixed values, one for cleaning the DataSeriesDatum types and one to add either values of the band to the legend.

# [3.10.0](v3.9.0...v3.10.0) (2019-04-11) ### Features * add band area chart ([#157](#157)) ([a9307ef](a9307ef)), closes [#144](#144)

markov00 · 2019-04-11T14:52:36Z

🎉 This PR is included in version 3.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [3.10.0](elastic/elastic-charts@v3.9.0...v3.10.0) (2019-04-11) ### Features * add band area chart ([opensearch-project#157](elastic/elastic-charts#157)) ([5a7c740](elastic/elastic-charts@5a7c740)), closes [opensearch-project#144](elastic/elastic-charts#144)

markov00 force-pushed the feat-band-area branch 2 times, most recently from bcb04db to 81812fa Compare April 9, 2019 20:49

markov00 marked this pull request as ready for review April 9, 2019 21:52

markov00 added wip work in progress :chart Chart element related issue enhancement New feature or request labels Apr 9, 2019

markov00 mentioned this pull request Apr 9, 2019

Roadmap #3

Closed

93 tasks

markov00 added 9 commits April 10, 2019 22:46

feat(bands): add band area chart

6fd24e2

Add a `y0Accessor` prop for each series to enable the possibility to add a lower limit to area and bars. close elastic#144

fix(tooltip): fix tooltip y0 rendering

3fa80d0

refactoring of the indexed geometries: removed the extra duplicated values in favour of a better rendering geometry.

docs(band): update band chart story

7424aae

refactor: return initialY values on GeometryValue

4fad230

fix: stacked bar tooltip with initial y1 value

2b14245

fix: missing accessor prop on geometry

06fa0c3

test: updated tests for tooltip

f332715

This commit also remove the wrongly conceived seriesKey with an undefined element in there. It's now a clean empty array if you wrongly specified a wrong split accessor

refactor: moved stack processing to external function

4c155fa

The logic of data structure in stacked charts is changed a bit to reflect correctly what are the initial values of the datum and what are the current stacked data

refactor: add comments and remove unnecessary type

8e0bfe7

markov00 force-pushed the feat-band-area branch from 6a01d0b to 8e0bfe7 Compare April 10, 2019 20:58

markov00 added 2 commits April 11, 2019 00:17

test: increase coverage for series and rendering bands

bdd54ac

test: fix missing updated test

c2156d9

emmacunningham approved these changes Apr 11, 2019

View reviewed changes

markov00 merged commit a9307ef into elastic:master Apr 11, 2019

markov00 pushed a commit that referenced this pull request Apr 11, 2019

chore(release): 3.10.0 [skip ci]

ad591c3

# [3.10.0](v3.9.0...v3.10.0) (2019-04-11) ### Features * add band area chart ([#157](#157)) ([a9307ef](a9307ef)), closes [#144](#144)

markov00 added the released Issue released publicly label Apr 11, 2019

markov00 deleted the feat-band-area branch April 16, 2019 12:03

markov00 mentioned this pull request Apr 18, 2019

Preparation for TSVB timeseries chart replacement #64

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add band area chart #157

feat: add band area chart #157

markov00 commented Apr 8, 2019 •

edited

codecov-io commented Apr 10, 2019 •

edited

emmacunningham left a comment •

edited

emmacunningham Apr 10, 2019

markov00 Apr 11, 2019

markov00 commented Apr 11, 2019

markov00 commented Apr 11, 2019

feat: add band area chart #157

feat: add band area chart #157

Conversation

markov00 commented Apr 8, 2019 • edited

Summary

Checklist

codecov-io commented Apr 10, 2019 • edited

Codecov Report

emmacunningham left a comment • edited

Choose a reason for hiding this comment

emmacunningham Apr 10, 2019

Choose a reason for hiding this comment

markov00 Apr 11, 2019

Choose a reason for hiding this comment

markov00 commented Apr 11, 2019

markov00 commented Apr 11, 2019

markov00 commented Apr 8, 2019 •

edited

codecov-io commented Apr 10, 2019 •

edited

emmacunningham left a comment •

edited