Skip to content
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(xy): stacked polarity #1502

Merged
merged 29 commits into from
Dec 9, 2021
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
6637aaf
fix: stacked polarity
nickofthyme Nov 23, 2021
de17519
refactor: stacking logic to group by x values
nickofthyme Dec 3, 2021
5b08ee4
feat: add diverging support for all offset types
nickofthyme Dec 8, 2021
0dcb91a
Merge branch 'master' into fix-stacked-polarity
nickofthyme Dec 8, 2021
56249bc
fix: block using y0 accessors with split accessors
nickofthyme Dec 8, 2021
ef38163
fix: add bypass for non-mixed polarity data
nickofthyme Dec 8, 2021
6f1cdb9
fix: remove filtered series from stack logic
nickofthyme Dec 8, 2021
c8eefea
fix: percentage stack offset to work for mixed polarity
nickofthyme Dec 8, 2021
96797fe
fix: wiggle for negative values
nickofthyme Dec 8, 2021
cf7ae39
fix: cleanup types for offset functions
nickofthyme Dec 8, 2021
a9e7486
test: add vrt for all offset options and polarity cases
nickofthyme Dec 8, 2021
0f40afc
fix: diverging logic for 0 case
nickofthyme Dec 8, 2021
fde8d5a
refactor: offset values to include y0 for fitting purposes
nickofthyme Dec 8, 2021
c1cf8ad
refactor: ignore y0Accessors when used with stackAccessors at the dat…
nickofthyme Dec 8, 2021
25d11af
fix: apply percentage domain logic only when stacked
nickofthyme Dec 8, 2021
8a1aef2
fix: remove y0 points in disallowed banded stack area charts
nickofthyme Dec 8, 2021
e1034d8
fix: series toggle logic with stacked offsets
nickofthyme Dec 8, 2021
0a58d76
fix: percent offset stopping after sum of zero
nickofthyme Dec 8, 2021
1b0a6a1
test: update vrt screenshots for mixes stories
nickofthyme Dec 8, 2021
ef547ea
fix: stacking order and stack value logic
nickofthyme Dec 8, 2021
7e9149a
test: add tests for found and fixed edge cases
nickofthyme Dec 8, 2021
075527b
test: prune stale vrt screenshots
nickofthyme Dec 8, 2021
d4c0196
fix: linting error
nickofthyme Dec 8, 2021
e916c6d
test: fix failing tests
nickofthyme Dec 9, 2021
9eb5bf4
feat: add 0 baseline datum
nickofthyme Dec 9, 2021
73a9e3c
refactor: rename banded spec check to be consistent throughout
nickofthyme Dec 9, 2021
ff2f537
test: update vrt screenshots with added baseline datum
nickofthyme Dec 9, 2021
f06db8a
chore: add warning when using area charts on mixed polarity datasets
nickofthyme Dec 9, 2021
edcb883
test: fix failing tests
nickofthyme Dec 9, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ coverage/
npm-debug.log
yarn-error.log
playground/playground.tsx
.jest-image-snapshot-touched-files
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved

**/__diff_output__/
2 changes: 1 addition & 1 deletion NOTICE.txt
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ THIS SOFTWARE.

---

This product includes code that is adapted from d3-array@3.0.4 and d3-scale@4.0.2,
This product includes code that is adapted from d3-shape@3.0.1, d3-array@3.0.4 and d3-scale@4.0.2,
which are both available under a "ISC" license.

ISC License
Expand Down
7 changes: 5 additions & 2 deletions integration/page_objects/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,12 @@ class CommonPage {
async expectChartWithMouseAtUrlToMatchScreenshot(
url: string,
mousePosition: MousePosition,
options?: Omit<ScreenshotElementAtUrlOptions, 'action'>,
options?: ScreenshotElementAtUrlOptions,
) {
const action = async () => await this.moveMouseRelativeToDOMElement(mousePosition, this.chartSelector);
const action = async () => {
await options?.action?.();
await this.moveMouseRelativeToDOMElement(mousePosition, this.chartSelector);
};
await this.expectChartAtUrlToMatchScreenshot(url, {
...options,
action,
Expand Down
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
Diff not rendered.
16 changes: 0 additions & 16 deletions integration/tests/area_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,6 @@ describe('Area series stories', () => {
});
});

describe('scale to extents', () => {
describe('domain.fit is true', () => {
const trueUrl = 'http://localhost:9001/?path=/story/area-chart--stacked-band&knob-fit Y domain=true';
it('should show correct extents - Banded', async () => {
await common.expectChartAtUrlToMatchScreenshot(trueUrl);
});
});

describe('domain.fit is false', () => {
const falseUrl = 'http://localhost:9001/?path=/story/area-chart--stacked-band&knob-fit Y domain=false';

it('should show correct extents - Banded', async () => {
await common.expectChartAtUrlToMatchScreenshot(falseUrl);
});
});
});
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
describe('Non-Stacked Linear Area with discontinuous data points', () => {
it('with fit', async () => {
await common.expectChartAtUrlToMatchScreenshot(
Expand Down
25 changes: 24 additions & 1 deletion integration/tests/mixed_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import { Fit } from '../../packages/charts/src';
import { Fit, StackMode, SeriesType } from '../../packages/charts/src';
import { common } from '../page_objects';

describe('Mixed series stories', () => {
Expand Down Expand Up @@ -195,4 +195,27 @@ describe('Mixed series stories', () => {
});
});
});

describe.each(Object.values(StackMode))('Stack mode - %s', (mode) => {
describe.each(['Mixed', 'Positive', 'Negative'])('Polarity - %s', (polarity) => {
describe.each([SeriesType.Bar, SeriesType.Area])('%s series', (type) => {
it('should display correct stacking', async () => {
await common.expectChartAtUrlToMatchScreenshot(
`http://localhost:9001/?path=/story/mixed-charts---polarized-stacked&globals=theme:light&knob-stacked=true&knob-data polarity=${polarity}&knob-custom domain=false&knob-stackMode=${mode}&knob-SeriesType=${type}`,
);
});

it('should show area chart with toggled series and mouse over', async () => {
const action = async () => {
await page.click('.echLegendItem:nth-child(2) .echLegendItem__label');
};
await common.expectChartWithMouseAtUrlToMatchScreenshot(
`http://localhost:9001/?path=/story/mixed-charts---polarized-stacked&globals=theme:light&knob-stacked=true&knob-data polarity=${polarity}&knob-custom domain=false&knob-stackMode=${mode}&knob-SeriesType=${type}`,
{ top: 170, left: 490 },
{ action },
);
});
});
});
});
});
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
"@types/d3-collection": "^1.0.8",
"@types/d3-interpolate": "^1.3.1",
"@types/d3-scale": "^2.1.1",
"@types/d3-shape": "^1.3.1",
"@types/d3-shape": "^2.0.0",
"@types/enzyme": "^3.9.0",
"@types/enzyme-adapter-react-16": "^1.0.5",
"@types/expect-puppeteer": "^4.4.5",
Expand Down
2 changes: 1 addition & 1 deletion packages/charts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"d3-collection": "^1.0.7",
"d3-interpolate": "^1.4.0",
"d3-scale": "^1.0.7",
"d3-shape": "^1.3.4",
"d3-shape": "^2.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was the issue on upgrading to the latest version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"prop-types": "^15.7.2",
"re-reselect": "^3.4.0",
"react-redux": "^7.1.0",
Expand Down
4 changes: 2 additions & 2 deletions packages/charts/src/chart_types/xy_chart/domains/y_domain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,14 @@ function mergeYDomainForGroup(
const dataSeries = [...stacked, ...nonStacked];
if (dataSeries.length === 0) return null;

const [{ stackMode, spec }] = dataSeries;
const [{ isStacked, stackMode, spec }] = dataSeries;
const groupId = getSpecDomainGroupId(spec);
const { customDomain, type, nice, desiredTickCount } = yScaleConfig[groupId];
const newCustomDomain: YDomainRange = customDomain ? { ...customDomain } : { min: NaN, max: NaN };
const { paddingUnit, padding, constrainPadding } = newCustomDomain;

let mergedDomain: ContinuousDomain;
if (stackMode === StackMode.Percentage) {
if (isStacked && stackMode === StackMode.Percentage) {
mergedDomain = computeContinuousDataDomain([0, 1], type, customDomain);
} else {
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
const stackedDomain = computeYDomain(stacked, hasZeroBaselineSpecs, type, newCustomDomain);
Expand Down
3 changes: 2 additions & 1 deletion packages/charts/src/chart_types/xy_chart/legend/legend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { mergePartial, Rotation } from '../../../utils/common';
import { BandedAccessorType } from '../../../utils/geometry';
import { getLegendCompareFn } from '../../../utils/series_sort';
import { PointStyle, Theme } from '../../../utils/themes/theme';
import { isStackedSpec } from '../domains/y_domain';
import { getXScaleTypeFromSpec } from '../scales/get_api_scales';
import { getAxesSpecForSpecId, getSpecsById } from '../state/utils/spec';
import { LastValues } from '../state/utils/types';
Expand Down Expand Up @@ -111,7 +112,7 @@ export function computeLegend(

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const key = getSeriesKey(series, series.groupId);
const spec = getSpecsById<BasicSeriesSpec>(specs, specId);
const dataSeriesKey = getSeriesKey(
Expand Down
7 changes: 4 additions & 3 deletions packages/charts/src/chart_types/xy_chart/rendering/area.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export function renderArea(
areaGeometry: AreaGeometry;
indexedGeometryMap: IndexedGeometryMap;
} {
const isBandChart = hasY0Accessors && !isStacked;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we reuse isDataSeriesBanded here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const y1Fn = getY1ScaledValueFn(yScale);
const y0Fn = getY0ScaledValueFn(yScale);
const definedFn = isYValueDefinedFn(yScale, xScale);
Expand All @@ -57,15 +58,15 @@ export function renderArea(
.y1(y1Fn)
.y0(y0Fn)
.defined((datum) => {
return definedFn(datum, y1DatumAccessor) && (hasY0Accessors ? definedFn(datum, y0DatumAccessor) : true);
return definedFn(datum, y1DatumAccessor) && (isBandChart ? definedFn(datum, y0DatumAccessor) : true);
})
.curve(getCurveFactory(curve));

// TODO we can probably avoid this function call if no fit function is applied.
const clippedRanges = getClippedRanges(dataSeries.data, xScale, xScaleOffset);

const lines: string[] = [];
const y0Line = hasY0Accessors && pathGenerator.lineY0()(dataSeries.data);
const y0Line = isBandChart && pathGenerator.lineY0()(dataSeries.data);
const y1Line = pathGenerator.lineY1()(dataSeries.data);
if (y1Line) lines.push(y1Line);
if (y0Line) lines.push(y0Line);
Expand All @@ -78,7 +79,7 @@ export function renderArea(
panel,
color,
style.point,
hasY0Accessors,
isBandChart,
markSizeOptions,
false,
pointStyleAccessor,
Expand Down
8 changes: 4 additions & 4 deletions packages/charts/src/chart_types/xy_chart/rendering/points.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export function renderPoints(
panel: Dimensions,
color: Color,
pointStyle: PointStyle,
hasY0Accessors: boolean,
isBandChart: boolean,
markSizeOptions: MarkSizeOptions,
useSpatialIndex: boolean,
styleAccessor?: PointStyleAccessor,
Expand Down Expand Up @@ -66,12 +66,12 @@ export function renderPoints(
if (Number.isNaN(x)) return acc;

const points: PointGeometry[] = [];
const yDatumKeyNames: Array<keyof Omit<FilledValues, 'x'>> = hasY0Accessors ? ['y0', 'y1'] : ['y1'];
const yDatumKeyNames: Array<keyof Omit<FilledValues, 'x'>> = isBandChart ? ['y0', 'y1'] : ['y1'];

yDatumKeyNames.forEach((yDatumKeyName, keyIndex) => {
const valueAccessor = getYDatumValueFn(yDatumKeyName);
const y = yDatumKeyName === 'y1' ? y1Fn(datum) : y0Fn(datum);
const originalY = getDatumYValue(datum, keyIndex === 0, hasY0Accessors, dataSeries.stackMode);
const originalY = getDatumYValue(datum, keyIndex === 0, isBandChart, dataSeries.stackMode);
const seriesIdentifier: XYChartSeriesIdentifier = {
key: dataSeries.key,
specId: dataSeries.specId,
Expand All @@ -98,7 +98,7 @@ export function renderPoints(
x: xValue,
y: originalY,
mark,
accessor: hasY0Accessors && keyIndex === 0 ? BandedAccessorType.Y0 : BandedAccessorType.Y1,
accessor: isBandChart && keyIndex === 0 ? BandedAccessorType.Y0 : BandedAccessorType.Y1,
datum: datum.datum,
},
transform: {
Expand Down
7 changes: 6 additions & 1 deletion packages/charts/src/chart_types/xy_chart/tooltip/tooltip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { TooltipValue } from '../../../specs';
import { getAccessorFormatLabel } from '../../../utils/accessor';
import { isDefined } from '../../../utils/common';
import { BandedAccessorType, IndexedGeometry } from '../../../utils/geometry';
import { isStackedSpec } from '../domains/y_domain';
import { defaultTickFormatter } from '../utils/axis_utils';
import { getSeriesName } from '../utils/series';
import {
Expand Down Expand Up @@ -66,7 +67,11 @@ export function formatTooltip(
): TooltipValue {
let label = getSeriesName(seriesIdentifier, hasSingleSeries, true, spec);

if (isBandedSpec(spec.y0Accessors) && (isAreaSeriesSpec(spec) || isBarSeriesSpec(spec))) {
if (
isBandedSpec(spec.y0Accessors) &&
!isStackedSpec(spec, false) &&
(isAreaSeriesSpec(spec) || isBarSeriesSpec(spec))
) {
const { y0AccessorFormat = Y0_ACCESSOR_POSTFIX, y1AccessorFormat = Y1_ACCESSOR_POSTFIX } = spec;
const formatter = accessor === BandedAccessorType.Y0 ? y0AccessorFormat : y1AccessorFormat;
label = getAccessorFormatLabel(formatter, label);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22663,7 +22663,7 @@ Array [
"initialY1": 3,
"mark": null,
"x": 1,
"y0": 1,
"y0": 0,
"y1": 3,
},
Object {
Expand All @@ -22678,7 +22678,7 @@ Array [
"initialY1": 3,
"mark": null,
"x": 2,
"y0": 2,
"y0": 0,
"y1": 3,
},
Object {
Expand All @@ -22705,7 +22705,7 @@ Array [
"initialY1": 4,
"mark": null,
"x": 4,
"y0": 3,
"y0": 0,
"y1": 4,
},
],
Expand Down Expand Up @@ -22734,7 +22734,7 @@ Array [
"initialY1": 2,
"mark": null,
"x": 1,
"y0": 4,
"y0": 3,
"y1": 5,
},
Object {
Expand All @@ -22749,7 +22749,7 @@ Array [
"initialY1": 3,
"mark": null,
"x": 2,
"y0": 4,
"y0": 3,
"y1": 6,
},
Object {
Expand All @@ -22764,7 +22764,7 @@ Array [
"initialY1": 23,
"mark": null,
"x": 3,
"y0": 4,
"y0": 0,
"y1": 23,
},
Object {
Expand All @@ -22779,7 +22779,7 @@ Array [
"initialY1": 4,
"mark": null,
"x": 4,
"y0": 5,
"y0": 4,
"y1": 8,
},
],
Expand Down Expand Up @@ -22813,7 +22813,7 @@ Array [
"initialY1": 3,
"mark": null,
"x": 1,
"y0": 1,
"y0": 0,
"y1": 3,
},
Object {
Expand All @@ -22828,7 +22828,7 @@ Array [
"initialY1": 3,
"mark": null,
"x": 2,
"y0": 2,
"y0": 0,
"y1": 3,
},
Object {
Expand All @@ -22855,7 +22855,7 @@ Array [
"initialY1": 4,
"mark": null,
"x": 4,
"y0": 3,
"y0": 0,
"y1": 4,
},
],
Expand Down Expand Up @@ -22884,7 +22884,7 @@ Array [
"initialY1": 2,
"mark": null,
"x": 1,
"y0": 4,
"y0": 3,
"y1": 5,
},
Object {
Expand All @@ -22899,7 +22899,7 @@ Array [
"initialY1": 3,
"mark": null,
"x": 2,
"y0": 4,
"y0": 3,
"y1": 6,
},
Object {
Expand All @@ -22914,7 +22914,7 @@ Array [
"initialY1": 23,
"mark": null,
"x": 3,
"y0": 4,
"y0": 0,
"y1": 23,
},
Object {
Expand All @@ -22929,7 +22929,7 @@ Array [
"initialY1": 4,
"mark": null,
"x": 4,
"y0": 5,
"y0": 4,
"y1": 8,
},
],
Expand Down
Loading