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: render orphan data points on lines and areas #900

Merged
merged 3 commits into from
Nov 12, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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.
7 changes: 7 additions & 0 deletions integration/tests/area_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,11 @@ describe('Area series stories', () => {
);
});
});
describe('Area with orphan data points', () => {
it('render correctly fit function', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/line-chart--test-orphan-data-points&knob-enable fit function=&knob-switch to area=true',
);
});
});
});
11 changes: 5 additions & 6 deletions src/chart_types/xy_chart/renderer/canvas/areas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,20 +74,19 @@ export function renderAreas(ctx: CanvasRenderingContext2D, props: AreaGeometries
});

areas.forEach(({ panel, value: area }) => {
const { seriesPointStyle, seriesIdentifier } = area;
if (!seriesPointStyle.visible) {
return;
}
const { seriesPointStyle, seriesIdentifier, points } = area;
const visiblePoints = seriesPointStyle.visible ? points : points.filter(({ orphan }) => orphan);

const geometryStateStyle = getGeometryStateStyle(seriesIdentifier, sharedStyle, highlightedLegendItem);
withPanelTransform(
ctx,
panel,
rotation,
renderingArea,
(ctx) => {
renderPoints(ctx, area.points, seriesPointStyle, geometryStateStyle);
renderPoints(ctx, visiblePoints, seriesPointStyle, geometryStateStyle);
},
{ area: clippings, shouldClip: area.points[0]?.value.mark !== null },
{ area: clippings, shouldClip: points[0]?.value.mark !== null },
);
});
});
Expand Down
29 changes: 14 additions & 15 deletions src/chart_types/xy_chart/renderer/canvas/lines.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,28 +46,27 @@ export function renderLines(ctx: CanvasRenderingContext2D, props: LineGeometries
const { lines, sharedStyle, highlightedLegendItem, clippings, renderingArea, rotation } = props;

lines.forEach(({ panel, value: line }) => {
const { seriesLineStyle, seriesPointStyle } = line;
const { seriesLineStyle, seriesPointStyle, points } = line;

if (seriesLineStyle.visible) {
withPanelTransform(ctx, panel, rotation, renderingArea, (ctx) => {
renderLine(ctx, line, sharedStyle, clippings, highlightedLegendItem);
});
}

if (seriesPointStyle.visible) {
withPanelTransform(
ctx,
panel,
rotation,
renderingArea,
(ctx) => {
const geometryStyle = getGeometryStateStyle(line.seriesIdentifier, sharedStyle, highlightedLegendItem);
renderPoints(ctx, line.points, line.seriesPointStyle, geometryStyle);
},
// TODO: add padding over clipping
{ area: clippings, shouldClip: line.points[0]?.value.mark !== null },
);
}
const visiblePoints = seriesPointStyle.visible ? points : points.filter(({ orphan }) => orphan);
withPanelTransform(
ctx,
panel,
rotation,
renderingArea,
(ctx) => {
const geometryStyle = getGeometryStateStyle(line.seriesIdentifier, sharedStyle, highlightedLegendItem);
renderPoints(ctx, visiblePoints, line.seriesPointStyle, geometryStyle);
},
// TODO: add padding over clipping
{ area: clippings, shouldClip: line.points[0]?.value.mark !== null },
);
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
});
});
}
Expand Down
5 changes: 4 additions & 1 deletion src/chart_types/xy_chart/rendering/area.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
getClippedRanges,
getY0ScaledValueOrThrow,
getY1ScaledValueOrThrow,
getYDatumValue,
isYValueDefined,
MarkSizeOptions,
} from './utils';
Expand Down Expand Up @@ -59,12 +60,14 @@ export function renderArea(
const y1Fn = getY1ScaledValueOrThrow(yScale);
const y0Fn = getY0ScaledValueOrThrow(yScale);
const definedFn = isYValueDefined(yScale, xScale);
const y1DatumAccessor = getYDatumValue();
const y0DatumAccessor = getYDatumValue('y0');
const pathGenerator = area<DataSeriesDatum>()
.x(({ x }) => xScale.scaleOrThrow(x) - xScaleOffset)
.y1(y1Fn)
.y0(y0Fn)
.defined((datum) => {
return definedFn(datum) && (hasY0Accessors ? definedFn(datum, 'y0') : true);
return definedFn(datum, y1DatumAccessor) && (hasY0Accessors ? definedFn(datum, y0DatumAccessor) : true);
})
.curve(getCurveFactory(curve));

Expand Down
5 changes: 3 additions & 2 deletions src/chart_types/xy_chart/rendering/line.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { IndexedGeometryMap } from '../utils/indexed_geometry_map';
import { DataSeries, DataSeriesDatum } from '../utils/series';
import { PointStyleAccessor } from '../utils/specs';
import { renderPoints } from './points';
import { getClippedRanges, getY1ScaledValueOrThrow, isYValueDefined, MarkSizeOptions } from './utils';
import { getClippedRanges, getY1ScaledValueOrThrow, getYDatumValue, isYValueDefined, MarkSizeOptions } from './utils';

/** @internal */
export function renderLine(
Expand All @@ -51,12 +51,13 @@ export function renderLine(
} {
const y1Fn = getY1ScaledValueOrThrow(yScale);
const definedFn = isYValueDefined(yScale, xScale);
const y1Accessor = getYDatumValue();

const pathGenerator = line<DataSeriesDatum>()
.x(({ x }) => xScale.scaleOrThrow(x) - xScaleOffset)
.y(y1Fn)
.defined((datum) => {
return definedFn(datum);
return definedFn(datum, y1Accessor);
})
.curve(getCurveFactory(curve));

Expand Down
40 changes: 35 additions & 5 deletions src/chart_types/xy_chart/rendering/points.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ import { PointStyleAccessor, StackMode } from '../utils/specs';
import {
getY0ScaledValueOrThrow,
getY1ScaledValueOrThrow,
getYDatumValue,
isDatumFilled,
isYValueDefined,
MarkSizeOptions,
YDefinedFn,
} from './utils';

/** @internal */
Expand Down Expand Up @@ -59,8 +61,10 @@ export function renderPoints(
const y0Fn = getY0ScaledValueOrThrow(yScale);
const yDefined = isYValueDefined(yScale, xScale);

const pointGeometries = dataSeries.data.reduce((acc, datum) => {
const pointGeometries = dataSeries.data.reduce((acc, datum, dataIndex) => {
const { x: xValue, mark } = datum;
const prev = dataSeries.data[dataIndex - 1];
const next = dataSeries.data[dataIndex + 1];
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
// don't create the point if not within the xScale domain
if (!xScale.isValueInDomain(xValue)) {
return acc;
Expand All @@ -78,7 +82,8 @@ export function renderPoints(
const points: PointGeometry[] = [];
const yDatumKeyNames: Array<keyof Omit<FilledValues, 'x'>> = hasY0Accessors ? ['y0', 'y1'] : ['y1'];

yDatumKeyNames.forEach((yDatumKeyName, index) => {
yDatumKeyNames.forEach((yDatumKeyName, keyIndex) => {
const valueAccessor = getYDatumValue(yDatumKeyName);
// skip rendering point if y1 is null
const radius = getRadius(mark);
let y: number | null;
Expand All @@ -91,7 +96,7 @@ export function renderPoints(
return;
}

const originalY = getDatumYValue(datum, index === 0, hasY0Accessors, dataSeries.stackMode);
const originalY = getDatumYValue(datum, keyIndex === 0, hasY0Accessors, dataSeries.stackMode);
const seriesIdentifier: XYChartSeriesIdentifier = {
key: dataSeries.key,
specId: dataSeries.specId,
Expand All @@ -102,6 +107,7 @@ export function renderPoints(
smHorizontalAccessorValue: dataSeries.smHorizontalAccessorValue,
};
const styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, styleAccessor);
const orphan = isOrphanData(prev, next, dataIndex, dataSeries.data.length, yDefined);
const pointGeometry: PointGeometry = {
radius,
x,
Expand All @@ -111,7 +117,7 @@ export function renderPoints(
x: xValue,
y: originalY,
mark,
accessor: hasY0Accessors && index === 0 ? BandedAccessorType.Y0 : BandedAccessorType.Y1,
accessor: hasY0Accessors && keyIndex === 0 ? BandedAccessorType.Y0 : BandedAccessorType.Y1,
datum: datum.datum,
},
transform: {
Expand All @@ -121,10 +127,11 @@ export function renderPoints(
seriesIdentifier,
styleOverrides,
panel,
orphan,
};
indexedGeometryMap.set(pointGeometry, geometryType);
// use the geometry only if the yDatum in contained in the current yScale domain
if (yDefined(datum, yDatumKeyName)) {
if (yDefined(datum, valueAccessor)) {
points.push(pointGeometry);
}
});
Expand Down Expand Up @@ -228,3 +235,26 @@ export function getRadiusFn(
return circleRadius ? Math.sqrt(circleRadius + baseMagicNumber) + lineWidth : lineWidth;
};
}

function yAccessorForOrphanCheck(datum: DataSeriesDatum): number | null {
return datum.filled?.y1 ? null : datum.y1;
}

function isOrphanData(
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
prev: DataSeriesDatum | undefined,
next: DataSeriesDatum | undefined,
index: number,
length: number,
yDefined: YDefinedFn,
) {
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
if (index === 0 && (next === undefined || !yDefined(next, yAccessorForOrphanCheck))) {
return true;
}
if (index === length - 1 && (prev === undefined || !yDefined(prev, yAccessorForOrphanCheck))) {
return true;
}
return (
(prev === undefined || !yDefined(prev, yAccessorForOrphanCheck)) &&
(next === undefined || !yDefined(next, yAccessorForOrphanCheck))
);
}
Loading