fix(xy): stacked polarity by nickofthyme · Pull Request #1502 · elastic/elastic-charts

nickofthyme · 2021-11-23T14:18:53Z

Summary

Resolves issue with stacking a mix of negative and positive bars and areas for any StackMode. Key changes include:

Disallowing stacked band charts (i.e. using stackAccessors with y0Accessors). Now shows console.warn when used and ignores y0Accessor for rendering but still adds to initialY0 value. See fix(xy): stacked polarity #1502 (comment).
Blocks showing banded legend and tooltip values for banded stacks, see case above.
Prevents percentage domain constraining when no stackAccessors are specified, see fix(xy): stacked polarity #1502 (comment)

No offset

Note: The original order of the series is maintained above and below 0.

Wiggle offset

Silhouette offset

Percentage offset

Note: The order is change for mixed values to displace negative values below positive values.

Details

Stacked offsets was implemented to be grouped by series. This creates issues when attempting to sort values differently based on their polarity for a given x value grouping. Thus this logic has been changed to group and stack by x values as demonstrated in this example, which allows for using d3.stackOffsetDiverging.

Additionally, the logic had an intermediate step to group by y0 or y1 values that is no longer needed. This is handled by using the y0 series as a placeholder during stacking and ignoring it when structuring the final geometry data. Though not advisable to stack when using y0Accessors.

Issues

fix #1280

Checklist

The proper chart type label has been added (e.g. :xy, :partition)
The proper feature labels have been added (e.g. :interactions, :axis)
All related issues have been linked (i.e. closes #123, fixes #123)
New public API exports have been added to packages/charts/src/index.ts
Unit tests have been added or updated to match the most common scenarios
The proper documentation and/or storybook story has been added or updated

packages/charts/package.json

…aseries level

packages/charts/src/chart_types/xy_chart/domains/y_domain.ts

storybook/stories/area/14_stacked_band.story.tsx

.gitignore

integration/tests/area_stories.test.ts

packages/charts/src/chart_types/xy_chart/utils/stacked_series_utils.ts

markov00 · 2021-12-09T10:03:25Z

packages/charts/package.json

    "d3-interpolate": "^1.4.0",
    "d3-scale": "^1.0.7",
-    "d3-shape": "^1.3.4",
+    "d3-shape": "^2.0.0",


what was the issue on upgrading to the latest version?

The jest tests were throwing an error similar to https://stackoverflow.com/questions/49263429/jest-gives-an-error-syntaxerror-unexpected-token-export. I tried to fix it but couldn't see any difference between the 2 so I gave up for now.

packages/charts/src/chart_types/xy_chart/domains/y_domain.ts

markov00 · 2021-12-09T10:09:18Z

packages/charts/src/chart_types/xy_chart/legend/legend.ts

  dataSeries.forEach((series) => {
    const { specId, yAccessor } = series;
-    const banded = isDataSeriesBanded(series);
+    const banded = isDataSeriesBanded(series) && !isStackedSpec(series.spec, false);


since isDataSeriesBanded is only used here and it name is really specific we can probably include the non-stacked check within that function so we have a clear and specific definition of what banded mean

markov00 · 2021-12-09T10:10:15Z

packages/charts/src/chart_types/xy_chart/rendering/area.ts

  areaGeometry: AreaGeometry;
  indexedGeometryMap: IndexedGeometryMap;
 } {
+  const isBandChart = hasY0Accessors && !isStacked;


can we reuse isDataSeriesBanded here?

markov00 · 2021-12-09T10:11:50Z

packages/charts/src/chart_types/xy_chart/utils/series.ts

  const dataSeries = new Map<SeriesKey, DataSeries>();
  const xValues: Array<string | number> = [];
  const nonNumericValues: any[] = [];
+  const hasStackedBands = isStacked && Boolean(y0Accessors?.length);


reuse isDataSeriesBanded

markov00 · 2021-12-09T12:50:01Z

storybook/stories/mixed/8_polarized_stacked.story.tsx

+      />
+    </Chart>


If you add a zero baseline it will look better in some cases:

<LineAnnotation id="zeroBaseline" domainType={AnnotationDomainType.YDomain} dataValues={[{ dataValue: 0 }]} style={{ line: { ...baseTheme.axes.axisLine, opacity: 1 } }} zIndex={-1} />

markov00 · 2021-12-09T12:51:17Z

storybook/stories/mixed/8_polarized_stacked.story.tsx

+  const polarity = select('data polarity', ['Mixed', 'Positive', 'Negative'], 'Mixed');
+  const customDomain = boolean('custom domain', false);
+  const stackMode =
+    select<StackMode | undefined>(
+      'stackMode',
+      {
+        None: undefined,
+        Silhouette: StackMode.Silhouette,
+        Wiggle: StackMode.Wiggle,
+        Percentage: StackMode.Percentage,
+      },
+      undefined,
+    ) ?? undefined;
+  const [Series] = getXYSeriesKnob('SeriesType', SeriesType.Bar, undefined, {
+    ignore: [SeriesType.Bubble, SeriesType.Line],
+  });


Area chart with mixed polarity values is not a good practice. Adding a warning on the console or as a text below the example can be useful.
I'm also not sure if it is worth having such cases in VRTs

A warning is good. I added the VRT thinking we could fix this in the future with the split series we had discussed. If you think that is not going to happen I can remove the case for mixed polarity stacked areas.

Added the warning in f06db8a. I'll keep the vrt for mixed area for now and we can decide if/when to support that later.

# [40.2.0](v40.1.0...v40.2.0) (2021-12-09) ### Bug Fixes * **partition:** linkLabel textColor override ([#1498](#1498)) ([3013310](3013310)) * **waffle:** use descend sortPredicate by default ([#1510](#1510)) ([763e2e3](763e2e3)) * **xy:** stacked polarity ([#1502](#1502)) ([920666a](920666a)), closes [#1280](#1280) ### Features * **xy:** expose style for interpolation fit functions ([#1505](#1505)) ([3071457](3071457))

Resolves issue with stacking a mix of negative and positive bars and areas for any `StackMode`. Key changes include: - Disallowing stacked band charts (i.e. using `stackAccessors` with `y0Accessors`). Now shows `console.warn` when used and ignores `y0Accessor` for rendering but still adds to `initialY0` value. - Blocks showing banded legend and tooltip values for banded stacks, see case above. - Prevents percentage domain constraining when no `stackAccessors` are specified. fix elastic#1280 # Conflicts: # packages/charts/src/chart_types/xy_chart/rendering/area.ts

Resolves issue with stacking a mix of negative and positive bars and areas for any `StackMode`. Key changes include: - Disallowing stacked band charts (i.e. using `stackAccessors` with `y0Accessors`). Now shows `console.warn` when used and ignores `y0Accessor` for rendering but still adds to `initialY0` value. - Blocks showing banded legend and tooltip values for banded stacks, see case above. - Prevents percentage domain constraining when no `stackAccessors` are specified. fix #1280

## [40.1.1](v40.1.0...v40.1.1) (2021-12-09) ### Bug Fixes * **partition:** linkLabel textColor override ([#1498](#1498)) ([#1523](#1523)) ([5324c76](5324c76)) * **xy:** stacked polarity ([#1502](#1502)) ([#1522](#1522)) ([bfb853f](bfb853f)), closes [#1280](#1280)

Resolves issue with stacking a mix of negative and positive bars and areas for any `StackMode`. Key changes include: - Disallowing stacked band charts (i.e. using `stackAccessors` with `y0Accessors`). Now shows `console.warn` when used and ignores `y0Accessor` for rendering but still adds to `initialY0` value. - Blocks showing banded legend and tooltip values for banded stacks, see case above. - Prevents percentage domain constraining when no `stackAccessors` are specified. fix elastic#1280 # Conflicts: # integration/tests/__image_snapshots__/axis-stories-test-ts-axis-stories-should-switch-to-a-30-minute-raster-1-snap.png # packages/charts/src/chart_types/xy_chart/rendering/area.ts # packages/charts/src/chart_types/xy_chart/utils/stacked_series_utils.ts

fix: stacked polarity

6637aaf

nickofthyme added bug Something isn't working :data Data/series/scales related issue :xy Bar/Line/Area chart related labels Nov 23, 2021

refactor: stacking logic to group by x values

de17519

nickofthyme changed the title ~~fix: stacked polarity~~ fix(xy): stacked polarity Dec 3, 2021

feat: add diverging support for all offset types

5b08ee4

nickofthyme mentioned this pull request Dec 8, 2021

feat: expose computeRatioByGroups fn #1495

Merged

6 tasks

Merge branch 'master' into fix-stacked-polarity

0dcb91a

nickofthyme commented Dec 8, 2021

View reviewed changes

packages/charts/package.json Outdated Show resolved Hide resolved

nickofthyme added 11 commits December 7, 2021 21:17

fix: block using y0 accessors with split accessors

56249bc

fix: add bypass for non-mixed polarity data

ef38163

fix: remove filtered series from stack logic

6f1cdb9

fix: percentage stack offset to work for mixed polarity

c8eefea

fix: wiggle for negative values

96797fe

fix: cleanup types for offset functions

cf7ae39

test: add vrt for all offset options and polarity cases

a9e7486

fix: diverging logic for 0 case

0f40afc

refactor: offset values to include y0 for fitting purposes

fde8d5a

refactor: ignore y0Accessors when used with stackAccessors at the dat…

c1cf8ad

…aseries level

fix: apply percentage domain logic only when stacked

25d11af

nickofthyme commented Dec 8, 2021

View reviewed changes

packages/charts/src/chart_types/xy_chart/domains/y_domain.ts Show resolved Hide resolved

nickofthyme added 4 commits December 8, 2021 13:45

fix: remove y0 points in disallowed banded stack area charts

8a1aef2

fix: series toggle logic with stacked offsets

e1034d8

fix: percent offset stopping after sum of zero

0a58d76

test: update vrt screenshots for mixes stories

1b0a6a1

nickofthyme commented Dec 8, 2021

View reviewed changes

storybook/stories/area/14_stacked_band.story.tsx Show resolved Hide resolved

nickofthyme added 3 commits December 8, 2021 16:39

fix: stacking order and stack value logic

ef547ea

test: add tests for found and fixed edge cases

7e9149a

test: prune stale vrt screenshots

075527b

nickofthyme commented Dec 8, 2021

View reviewed changes

.gitignore Show resolved Hide resolved

nickofthyme commented Dec 8, 2021

View reviewed changes

integration/tests/area_stories.test.ts Show resolved Hide resolved

nickofthyme requested a review from markov00 December 8, 2021 22:55

nickofthyme marked this pull request as ready for review December 8, 2021 22:56

nickofthyme added 2 commits December 8, 2021 17:19

fix: linting error

d4c0196

test: fix failing tests

e916c6d

nickofthyme commented Dec 9, 2021

View reviewed changes

packages/charts/src/chart_types/xy_chart/utils/stacked_series_utils.ts Outdated Show resolved Hide resolved

markov00 reviewed Dec 9, 2021

View reviewed changes

markov00 approved these changes Dec 9, 2021

View reviewed changes

nickofthyme added 5 commits December 9, 2021 11:06

feat: add 0 baseline datum

9eb5bf4

refactor: rename banded spec check to be consistent throughout

73a9e3c

test: update vrt screenshots with added baseline datum

ff2f537

chore: add warning when using area charts on mixed polarity datasets

f06db8a

test: fix failing tests

edcb883

nickofthyme merged commit 920666a into elastic:master Dec 9, 2021

nickofthyme deleted the fix-stacked-polarity branch December 9, 2021 20:55

nickofthyme mentioned this pull request Dec 9, 2021

fix(xy): stacked polarity (#1502) [40.1.x] #1522

Merged

nickofthyme mentioned this pull request Dec 10, 2021

fix(xy): stacked polarity (#1502) [38.1.x] #1526

Merged

Conversation

nickofthyme commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

No offset

Wiggle offset

Silhouette offset

Percentage offset

Details

Issues

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickofthyme Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nickofthyme commented Nov 23, 2021 •

edited

Loading

nickofthyme Dec 9, 2021 •

edited

Loading