fix(rendering): area, line and point rendered on the same layer for each series #290

markov00 · 2019-08-06T09:50:28Z

Summary

This commit change the rendering order of the line series and area series.
For lines we render: for each series the line and each data point, instead of rendering first all the lines and than all the points.
For areas, depending if the area is stacked or not:

if stacked, we render fist all the areas, than all the lines and than all the points (this will avoid having a lines partially rendered below the above area)
if non stacked, we render groups of areas, line, points. so each group id rendered on a different layer.

This PR also remove completely the animation from lines and areas as its currently disabled in the library. We will reintegrate the animation in a future release.

fix #287

before

after

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

This commit change the rendering order of the line series. Now we render: for each series the line and each data point, instead of rendering first all the lines and than all the points fix elastic#287

codecov-io · 2019-08-06T09:55:20Z

Codecov Report

Merging #290 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
- Coverage   98.17%   98.14%   -0.03%     
==========================================
  Files          37       37              
  Lines        2682     3019     +337     
  Branches      617      759     +142     
==========================================
+ Hits         2633     2963     +330     
- Misses         44       49       +5     
- Partials        5        7       +2

Impacted Files	Coverage Δ
src/chart_types/xy_chart/store/utils.ts	`96.55% <ø> (-0.13%)`	⬇️
src/chart_types/xy_chart/rendering/rendering.ts	`98.81% <100%> (-0.51%)`	⬇️
src/chart_types/xy_chart/legend/legend.ts	`100% <0%> (ø)`	⬆️
src/chart_types/xy_chart/utils/series.ts	`100% <0%> (ø)`	⬆️
src/chart_types/xy_chart/utils/specs.ts	`100% <0%> (ø)`	⬆️
src/chart_types/xy_chart/store/chart_state.ts	`97.48% <0%> (+0.02%)`	⬆️

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 759e8b5...c4fffd9. Read the comment docs.

nickofthyme · 2019-08-06T15:05:44Z

src/components/react_canvas/line_geometries.tsx

+
+  getLineToRender(glyph: LineGeometry, sharedStyle: SharedGeometryStyle, index: number) {
+    const { line, color, transform, geometryId, seriesLineStyle } = glyph;
+    const key = `line-${index}`;


Can we use something other than the index for the key?

fixed on 5d712a5

nickofthyme · 2019-08-06T15:06:16Z

src/components/react_canvas/line_geometries.tsx

+
+    if (this.props.animated) {
+      return (
+        <Group key={index} x={transform.x}>


Also here? re: index

fixed on 5d712a5

nickofthyme · 2019-08-06T15:09:04Z

src/components/react_canvas/line_geometries.tsx

+      return (
+        <Group key={index} x={transform.x}>
+          <Spring native reset from={{ opacity: 0 }} to={{ opacity: 1 }}>
+            {() => {


Can we extract this function so it doesn't create a new function on each rerender? I don't think it will be that different on performance but best to keep up with best practices.

removed the animation in 5d712a5

Depending if stacked or not, we draw first all the stacked areas (fist all the areas, then lines then points). For non stacked elements, we draw groups of area, line, points.

nickofthyme

This looks a lot better. I think removing the animations is a good idea for now.

Just have a few comments about the index in the string. I think including that in the key string will cause the component to rerender if the index value changes in the future, at least that's my understanding of react. If the key would be unique without the index I think it should be removed.

src/chart_types/xy_chart/rendering/rendering.ts

src/components/react_canvas/line_geometries.tsx

src/components/react_canvas/area_geometries.tsx

nickofthyme

👍

## [9.0.3](v9.0.2...v9.0.3) (2019-08-13) ### Bug Fixes * zIndex order for areas, lines and points ([#290](#290)) ([6a4c1b1](6a4c1b1)), closes [#287](#287)

markov00 · 2019-08-13T11:46:28Z

🎉 This PR is included in version 9.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

## [9.0.3](elastic/elastic-charts@v9.0.2...v9.0.3) (2019-08-13) ### Bug Fixes * zIndex order for areas, lines and points ([opensearch-project#290](elastic/elastic-charts#290)) ([4552c45](elastic/elastic-charts@4552c45)), closes [opensearch-project#287](elastic/elastic-charts#287)

fix(lines): line and point rendered on the same layer for each series

17458ea

This commit change the rendering order of the line series. Now we render: for each series the line and each data point, instead of rendering first all the lines and than all the points fix elastic#287

markov00 added bug Something isn't working :chart Chart element related issue labels Aug 6, 2019

markov00 requested review from emmacunningham and nickofthyme August 6, 2019 09:50

nickofthyme reviewed Aug 6, 2019

View reviewed changes

markov00 mentioned this pull request Aug 9, 2019

Replace TSVB timeseries charts with elastic-charts elastic/kibana#33558

Merged

55 tasks

refactor(areas): fix areas rendering order

f612276

Depending if stacked or not, we draw first all the stacked areas (fist all the areas, then lines then points). For non stacked elements, we draw groups of area, line, points.

markov00 changed the title ~~fix(lines): line and point rendered on the same layer for each series~~ fix(rendering): area, line and point rendered on the same layer for each series Aug 9, 2019

refactor(line): fix line keys and remove springs

5d712a5

nickofthyme approved these changes Aug 9, 2019

View reviewed changes

refactor: rename getGeometryId _AsString to _Key

c4fffd9

nickofthyme approved these changes Aug 12, 2019

View reviewed changes

markov00 merged commit 6a4c1b1 into elastic:master Aug 13, 2019

markov00 deleted the fix_rendering_order_line branch August 13, 2019 09:55

markov00 pushed a commit that referenced this pull request Aug 13, 2019

chore(release): 9.0.3 [skip ci]

e11697f

## [9.0.3](v9.0.2...v9.0.3) (2019-08-13) ### Bug Fixes * zIndex order for areas, lines and points ([#290](#290)) ([6a4c1b1](6a4c1b1)), closes [#287](#287)

markov00 added the released Issue released publicly label Aug 13, 2019

alexwizp mentioned this pull request Aug 19, 2019

Update @elastic/charts version 8.1.6 > 9.1.1 elastic/kibana#43516

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rendering): area, line and point rendered on the same layer for each series #290

fix(rendering): area, line and point rendered on the same layer for each series #290

markov00 commented Aug 6, 2019 •

edited

Loading

codecov-io commented Aug 6, 2019 •

edited

Loading

nickofthyme Aug 6, 2019

markov00 Aug 9, 2019

nickofthyme Aug 6, 2019 •

edited

Loading

markov00 Aug 9, 2019

nickofthyme Aug 6, 2019

markov00 Aug 9, 2019

nickofthyme left a comment

nickofthyme left a comment

markov00 commented Aug 13, 2019

fix(rendering): area, line and point rendered on the same layer for each series #290

fix(rendering): area, line and point rendered on the same layer for each series #290

Conversation

markov00 commented Aug 6, 2019 • edited Loading

Summary

Checklist

codecov-io commented Aug 6, 2019 • edited Loading

Codecov Report

nickofthyme Aug 6, 2019

Choose a reason for hiding this comment

markov00 Aug 9, 2019

Choose a reason for hiding this comment

nickofthyme Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

markov00 Aug 9, 2019

Choose a reason for hiding this comment

nickofthyme Aug 6, 2019

Choose a reason for hiding this comment

markov00 Aug 9, 2019

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

markov00 commented Aug 13, 2019

markov00 commented Aug 6, 2019 •

edited

Loading

codecov-io commented Aug 6, 2019 •

edited

Loading

nickofthyme Aug 6, 2019 •

edited

Loading