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 all commits
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',
);
});
});
});
9 changes: 5 additions & 4 deletions src/chart_types/xy_chart/renderer/canvas/areas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,9 @@ export function renderAreas(ctx: CanvasRenderingContext2D, props: AreaGeometries
});

areas.forEach(({ panel, value: area }) => {
const { seriesPointStyle, seriesIdentifier } = area;
if (!seriesPointStyle.visible) {
const { seriesPointStyle, seriesIdentifier, points } = area;
const visiblePoints = seriesPointStyle.visible ? points : points.filter(({ orphan }) => orphan);
if (visiblePoints.length === 0) {
return;
}
const geometryStateStyle = getGeometryStateStyle(seriesIdentifier, sharedStyle, highlightedLegendItem);
Expand All @@ -85,9 +86,9 @@ export function renderAreas(ctx: CanvasRenderingContext2D, props: AreaGeometries
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
3 changes: 3 additions & 0 deletions src/chart_types/xy_chart/renderer/canvas/bars.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ function renderPerPanelBars(
rotation: Rotation = 0,
) {
return ({ panel, value: bars }: PerPanel<BarGeometry[]>) => {
if (bars.length === 0) {
return;
}
withPanelTransform(
ctx,
panel,
Expand Down
3 changes: 3 additions & 0 deletions src/chart_types/xy_chart/renderer/canvas/bubbles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ export function renderBubbles(ctx: CanvasRenderingContext2D, props: BubbleGeomet
},
[],
);
if (allPoints.length === 0) {
return;
}

renderPointGroup(
ctx,
Expand Down
30 changes: 16 additions & 14 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,30 @@ 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);
if (visiblePoints.length === 0) {
return;
}
const geometryStyle = getGeometryStateStyle(line.seriesIdentifier, sharedStyle, highlightedLegendItem);
withPanelTransform(
ctx,
panel,
rotation,
renderingArea,
(ctx) => {
renderPoints(ctx, visiblePoints, line.seriesPointStyle, geometryStyle);
},
// TODO: add padding over clipping
{ area: clippings, shouldClip: line.points[0]?.value.mark !== null },
);
});
});
}
Expand Down
17 changes: 10 additions & 7 deletions src/chart_types/xy_chart/rendering/area.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ import { PointStyleAccessor } from '../utils/specs';
import { renderPoints } from './points';
import {
getClippedRanges,
getY0ScaledValueOrThrow,
getY1ScaledValueOrThrow,
isYValueDefined,
getY0ScaledValueOrThrowFn,
getY1ScaledValueOrThrowFn,
getYDatumValueFn,
isYValueDefinedFn,
MarkSizeOptions,
} from './utils';

Expand All @@ -56,15 +57,17 @@ export function renderArea(
areaGeometry: AreaGeometry;
indexedGeometryMap: IndexedGeometryMap;
} {
const y1Fn = getY1ScaledValueOrThrow(yScale);
const y0Fn = getY0ScaledValueOrThrow(yScale);
const definedFn = isYValueDefined(yScale, xScale);
const y1Fn = getY1ScaledValueOrThrowFn(yScale);
const y0Fn = getY0ScaledValueOrThrowFn(yScale);
const definedFn = isYValueDefinedFn(yScale, xScale);
const y1DatumAccessor = getYDatumValueFn();
const y0DatumAccessor = getYDatumValueFn('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
15 changes: 11 additions & 4 deletions src/chart_types/xy_chart/rendering/line.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,13 @@ 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,
getY1ScaledValueOrThrowFn,
getYDatumValueFn,
isYValueDefinedFn,
MarkSizeOptions,
} from './utils';

/** @internal */
export function renderLine(
Expand All @@ -49,14 +55,15 @@ export function renderLine(
lineGeometry: LineGeometry;
indexedGeometryMap: IndexedGeometryMap;
} {
const y1Fn = getY1ScaledValueOrThrow(yScale);
const definedFn = isYValueDefined(yScale, xScale);
const y1Fn = getY1ScaledValueOrThrowFn(yScale);
const definedFn = isYValueDefinedFn(yScale, xScale);
const y1Accessor = getYDatumValueFn();

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
54 changes: 42 additions & 12 deletions src/chart_types/xy_chart/rendering/points.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,21 @@
* under the License.
*/
import { Scale } from '../../../scales';
import { Color } from '../../../utils/commons';
import { Color, isNil } from '../../../utils/commons';
import { Dimensions } from '../../../utils/dimensions';
import { BandedAccessorType, PointGeometry } from '../../../utils/geometry';
import { LineStyle, PointStyle } from '../../../utils/themes/theme';
import { GeometryType, IndexedGeometryMap } from '../utils/indexed_geometry_map';
import { DataSeries, DataSeriesDatum, FilledValues, XYChartSeriesIdentifier } from '../utils/series';
import { PointStyleAccessor, StackMode } from '../utils/specs';
import {
getY0ScaledValueOrThrow,
getY1ScaledValueOrThrow,
getY0ScaledValueOrThrowFn,
getY1ScaledValueOrThrowFn,
getYDatumValueFn,
isDatumFilled,
isYValueDefined,
isYValueDefinedFn,
MarkSizeOptions,
YDefinedFn,
} from './utils';

/** @internal */
Expand All @@ -55,12 +57,14 @@ export function renderPoints(
: () => 0;
const geometryType = spatial ? GeometryType.spatial : GeometryType.linear;

const y1Fn = getY1ScaledValueOrThrow(yScale);
const y0Fn = getY0ScaledValueOrThrow(yScale);
const yDefined = isYValueDefined(yScale, xScale);
const y1Fn = getY1ScaledValueOrThrowFn(yScale);
const y0Fn = getY0ScaledValueOrThrowFn(yScale);
const yDefined = isYValueDefinedFn(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 = getYDatumValueFn(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 = isOrphanDataPoint(dataIndex, dataSeries.data.length, yDefined, prev, next);
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 isOrphanDataPoint(
index: number,
length: number,
yDefined: YDefinedFn,
prev?: DataSeriesDatum,
next?: DataSeriesDatum,
): boolean {
if (index === 0 && (isNil(next) || !yDefined(next, yAccessorForOrphanCheck))) {
return true;
}
if (index === length - 1 && (isNil(prev) || !yDefined(prev, yAccessorForOrphanCheck))) {
return true;
}
return (
(isNil(prev) || !yDefined(prev, yAccessorForOrphanCheck)) &&
(isNil(next) || !yDefined(next, yAccessorForOrphanCheck))
);
}
Loading