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.

Sign up for GitHub

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

Jump to bottom

fix: disable x filling for line and areas #833

Merged
merged 3 commits into from
Sep 29, 2020
Merged

fix: disable x filling for line and areas #833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
45e45bb
fix: disable x filling for line and areas
markov00 Sep 23, 2020
36860dd
fix: last values on non-x-filled series
markov00 Sep 24, 2020
ae6fe1e
test: add discontinuity story
markov00 Sep 24, 2020
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
30 changes: 9 additions & 21 deletions .playground/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,29 +7,19 @@
<meta http-equiv="X-UA-Compatible" content="ie=edge" />
<style>
html,
body {
body,
#root {
background: blanchedalmond !important;
width: 100%;
height: 100%;
padding: 10px;
}

.chart {
background: white;
/*display: inline-block;
position: relative;
*/
width: 500px;
height: 200px;
overflow: auto;
}

.testing {
background: aquamarine;
position: relative;
width: 100%;
overflow: auto;
}

.page {
padding: 10px;
background: white;
width: 800px;
height: 500px;
}

label {
Expand All @@ -39,9 +29,7 @@
</head>

<body>
<div class="page">
<div id="root"></div>
</div>
<div id="root"></div>
<script src="bundle.js" type="text/javascript"></script>
</body>
</html>
16 changes: 7 additions & 9 deletions .playground/playground.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,12 @@

import React from 'react';

import { Example } from '../stories/sunburst/15_single_sunburst';
import { Example } from '../stories/bar/1_basic';

export class Playground extends React.Component {
render() {
return (
<div className="testing">
<div className="chart">{Example()}</div>
</div>
);
}
export function Playground() {
return (
<div className="chart">
<Example />
</div>
);
}
Binary file added ...-stories-line-chart-discontinuous-data-points-visually-looks-correct-1-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...n-stacked-linear-area-with-discontinuous-data-points-no-fit-function-1-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...ries-non-stacked-linear-area-with-discontinuous-data-points-with-fit-1-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...n-stacked-linear-line-with-discontinuous-data-points-no-fit-function-1-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...ries-non-stacked-linear-line-with-discontinuous-data-points-with-fit-1-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
13 changes: 13 additions & 0 deletions integration/tests/area_stories.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,17 @@ describe('Area series stories', () => {
});
});
});
describe('Non-Stacked Linear Area with discontinuous data points', () => {
it('with fit', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/line-chart--discontinuous-data-points&knob-enable fit function=false&knob-switch to area=true',
);
});

it('no fit function', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/line-chart--discontinuous-data-points&knob-enable fit function=true&knob-switch to area=true',
);
});
});
});
13 changes: 13 additions & 0 deletions integration/tests/line_stories.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,17 @@ describe('Line series stories', () => {
);
});
});
describe('Non-Stacked Linear Line with discontinuous data points', () => {
it('with fit', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/line-chart--discontinuous-data-points&knob-enable fit function=false&knob-switch to area=',
);
});

it('no fit function', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/line-chart--discontinuous-data-points&knob-enable fit function=true&knob-switch to area=',
);
});
});
markov00 marked this conversation as resolved.
Show resolved Hide resolved
});
50 changes: 39 additions & 11 deletions src/chart_types/xy_chart/state/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { GroupId, SpecId } from '../../../../utils/ids';
import { ColorConfig, Theme } from '../../../../utils/themes/theme';
import { XDomain, YDomain } from '../../domains/types';
import { mergeXDomain } from '../../domains/x_domain';
import { mergeYDomain } from '../../domains/y_domain';
import { mergeYDomain, splitSpecsByGroupId } from '../../domains/y_domain';
import { renderArea, renderBars, renderLine, renderBubble, isDatumFilled } from '../../rendering/rendering';
import { defaultTickFormatter } from '../../utils/axis_utils';
import { fillSeries } from '../../utils/fill_series';
Expand Down Expand Up @@ -131,18 +131,24 @@ export function getCustomSeriesColors(
return updatedCustomSeriesColors;
}

function getLastValues(formattedDataSeries: {
stacked: FormattedDataSeries[];
nonStacked: FormattedDataSeries[];
}): Map<SeriesKey, LastValues> {
function getLastValues(
formattedDataSeries: {
stacked: FormattedDataSeries[];
nonStacked: FormattedDataSeries[];
},
xDomain: XDomain,
): Map<SeriesKey, LastValues> {
const lastValues = new Map<SeriesKey, LastValues>();

if (xDomain.scaleType === ScaleType.Ordinal) {
return lastValues;
}
// we need to get the latest
formattedDataSeries.stacked.forEach(({ dataSeries, stackMode }) => {
dataSeries.forEach((series) => {
if (series.data.length === 0) {
return;
}

const last = series.data[series.data.length - 1];
if (!last) {
return;
Expand All @@ -151,6 +157,13 @@ function getLastValues(formattedDataSeries: {
return;
}

if (last.x !== xDomain.domain[xDomain.domain.length - 1]) {
// we have a dataset that is not filled with all x values
// and the last value of the series is not the last value for every series
// let's skip it
return;
}

const { y0, y1, initialY0, initialY1 } = last;
const seriesKey = getSeriesKey(series as XYChartSeriesIdentifier);

Expand Down Expand Up @@ -178,6 +191,13 @@ function getLastValues(formattedDataSeries: {
return;
}

if (last.x !== xDomain.domain[xDomain.domain.length - 1]) {
// we have a dataset that is not filled with all x values
// and the last value of the series is not the last value for every series
// let's skip it
return;
}

const { initialY1, initialY0 } = last;
const seriesKey = getSeriesKey(series as XYChartSeriesIdentifier);

Expand All @@ -194,6 +214,7 @@ function getLastValues(formattedDataSeries: {
* @param customXDomain if specified in <Settings />, the custom X domain
* @param deselectedDataSeries is optional; if not supplied,
* @param customXDomain is optional; if not supplied,
* @param orderOrdinalBinsBy
* @param enableVislibSeriesSort is optional; if not specified in <Settings />,
* then all series will be factored into computations. Otherwise, selectedDataSeries
* is used to restrict the computation for just the selected series
Expand All @@ -215,26 +236,33 @@ export function computeSeriesDomains(
enableVislibSeriesSort,
);
// compute the x domain merging any custom domain
const specsArray = [...seriesSpecs.values()];
const xDomain = mergeXDomain(specsArray, xValues, customXDomain, fallbackScale);
const xDomain = mergeXDomain(seriesSpecs, xValues, customXDomain, fallbackScale);

const specsByGroupIds = splitSpecsByGroupId(seriesSpecs);

// fill series with missing x values
const filledDataSeriesBySpecId = fillSeries(dataSeriesBySpecId, xValues);
const filledDataSeriesBySpecId = fillSeries(
dataSeriesBySpecId,
xValues,
seriesSpecs,
xDomain.scaleType,
specsByGroupIds,
);

const formattedDataSeries = getFormattedDataseries(
specsArray,
filledDataSeriesBySpecId,
xValues,
xDomain.scaleType,
seriesSpecs,
specsByGroupIds,
);

// let's compute the yDomain after computing all stacked values
const yDomain = mergeYDomain(formattedDataSeries, seriesSpecs, customYDomainsByGroupId);

// we need to get the last values from the formatted dataseries
// because we change the format if we are on percentage mode
const lastValues = xDomain.scaleType !== ScaleType.Ordinal ? getLastValues(formattedDataSeries) : new Map();
const lastValues = getLastValues(formattedDataSeries, xDomain);
const updatedSeriesCollection = new Map<SeriesKey, SeriesCollectionValue>();
seriesCollection.forEach((value, key) => {
const lastValue = lastValues.get(key);
Expand Down
37 changes: 33 additions & 4 deletions src/chart_types/xy_chart/utils/fill_series.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,47 @@
* specific language governing permissions and limitations
* under the License.
*/
import { SpecId } from '../../../utils/ids';
import { ScaleType } from '../../../scales/constants';
import { SpecId, GroupId } from '../../../utils/ids';
import { YBasicSeriesSpec } from '../domains/y_domain';
import { getSpecsById } from '../state/utils/spec';
import { DataSeries } from './series';
import { SeriesSpecs, StackMode, BasicSeriesSpec, isLineSeriesSpec, isAreaSeriesSpec } from './specs';

/**
* Fill missing x values in all data series
* @param series
* @param xValues
* @internal
*/
export function fillSeries(
series: Map<SpecId, DataSeries[]>,
xValues: Set<string | number>,
seriesSpecs: SeriesSpecs,
groupScaleType: ScaleType,
specsByGroupIds: Map<
GroupId,
{
stackMode: StackMode | undefined;
stacked: YBasicSeriesSpec[];
nonStacked: YBasicSeriesSpec[];
}
>,
): Map<SpecId, DataSeries[]> {
const sortedXValues = [...xValues.values()];
const filledSeries: Map<SpecId, DataSeries[]> = new Map();
series.forEach((dataSeries, key) => {
const spec = getSpecsById(seriesSpecs, key);
if (!spec) {
return;
}
const group = specsByGroupIds.get(spec.groupId);
if (!group) {
return;
}
const isStacked = Boolean(group.stacked.find(({ id }) => id === key));
const noFillRequired = isXFillNotRequired(spec, groupScaleType, isStacked);

const filledDataSeries = dataSeries.map(({ data, ...rest }) => {
if (data.length === xValues.size) {
if (data.length === xValues.size || noFillRequired) {
return {
...rest,
data,
Expand Down Expand Up @@ -74,3 +97,9 @@ export function fillSeries(
});
return filledSeries;
}

function isXFillNotRequired(spec: BasicSeriesSpec, groupScaleType: ScaleType, isStacked: boolean) {
const onlyNoFitAreaLine = (isAreaSeriesSpec(spec) || isLineSeriesSpec(spec)) && !spec.fit;
const onlyContinuous = groupScaleType === ScaleType.Linear || groupScaleType === ScaleType.Time;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this affect Ordinal scales? Do we not need to fit with Ordinal scales?

Copy link
Member Author

Choose a reason for hiding this comment

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

with Ordinal scales it should work as before. the line, usually indicates a continuity on the data, using ordinal aka categorical scale the continuity is not the primary thing.
Let's stick with that for the moment, if there is the case to link discontinued points also on categorical scales we can fix that later, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that's fine. The fitting function logic does account for ordinal values, I was just wondering if this eliminated that functionality. Looks like it is still working as it was....

https://github.com/elastic/elastic-charts/blob/ae6fe1e773818efed7a0d6a4cfbcd660363b46d4/integration/tests/__image_snapshots__/mixed-stories-test-ts-mixed-series-stories-fitting-functions-area-charts-ordinal-dataset-no-end-value-should-display-correct-fit-for-type-carry-1-snap.png

return onlyNoFitAreaLine && onlyContinuous && !isStacked;
}
10 changes: 6 additions & 4 deletions src/chart_types/xy_chart/utils/nonstacked_series_utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,10 @@ describe('Non-Stacked Series Utils', () => {
} = computeSeriesDomainsSelector(store.getState());

expect(dataSeries.length).toBe(2);
expect(dataSeries[0].data.length).toBe(4);
expect(dataSeries[1].data.length).toBe(4);
// this because linear non stacked area/lines doesn't fill up the dataset
// with missing x data points
expect(dataSeries[0].data.length).toBe(3);
expect(dataSeries[1].data.length).toBe(2);

expect(dataSeries[0].data[0]).toMatchObject({
initialY0: null,
Expand All @@ -245,7 +247,7 @@ describe('Non-Stacked Series Utils', () => {
y1: 2,
mark: null,
});
expect(dataSeries[0].data[3]).toMatchObject({
expect(dataSeries[0].data[2]).toMatchObject({
initialY0: null,
initialY1: 4,
x: 4,
Expand All @@ -261,7 +263,7 @@ describe('Non-Stacked Series Utils', () => {
y1: 21,
mark: null,
});
expect(dataSeries[1].data[2]).toMatchObject({
expect(dataSeries[1].data[1]).toMatchObject({
initialY0: null,
initialY1: 23,
x: 3,
Expand Down
7 changes: 5 additions & 2 deletions src/chart_types/xy_chart/utils/series.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { AccessorFn } from '../../../utils/accessor';
import { Position } from '../../../utils/commons';
import * as TestDataset from '../../../utils/data_samples/test_dataset';
import { ColorConfig } from '../../../utils/themes/theme';
import { splitSpecsByGroupId } from '../domains/y_domain';
import { computeSeriesDomainsSelector } from '../state/selectors/compute_series_domains';
import {
SeriesCollectionValue,
Expand Down Expand Up @@ -485,12 +486,14 @@ describe('Series', () => {
};
const xValues = new Set([0, 1, 2, 3]);
const splittedDataSeries = getDataSeriesBySpecId([spec1, spec2]);
const specsByGroupIds = splitSpecsByGroupId([spec1, spec2]);

const stackedDataSeries = getFormattedDataseries(
[spec1, spec2],
splittedDataSeries.dataSeriesBySpecId,
xValues,
ScaleType.Linear,
[],
[spec1, spec2],
specsByGroupIds,
);
expect(stackedDataSeries.stacked).toMatchSnapshot();
});
Expand Down
16 changes: 10 additions & 6 deletions src/chart_types/xy_chart/utils/series.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { Datum, Color } from '../../../utils/commons';
import { GroupId, SpecId } from '../../../utils/ids';
import { Logger } from '../../../utils/logger';
import { ColorConfig } from '../../../utils/themes/theme';
import { splitSpecsByGroupId, YBasicSeriesSpec } from '../domains/y_domain';
import { YBasicSeriesSpec } from '../domains/y_domain';
import { LastValues } from '../state/utils/types';
import { applyFitFunctionToDataSeries } from './fit_function_utils';
import { BasicSeriesSpec, SeriesTypes, SeriesSpecs, SeriesNameConfigOptions, StackMode } from './specs';
Expand Down Expand Up @@ -347,18 +347,22 @@ const getSortedDataSeries = (

/** @internal */
export function getFormattedDataseries(
specs: YBasicSeriesSpec[],
availableDataSeries: Map<SpecId, DataSeries[]>,
xValues: Set<string | number>,
xScaleType: ScaleType,
seriesSpecs: SeriesSpecs,
specsByGroupIdsEntries: Map<
GroupId,
{
stackMode: StackMode | undefined;
stacked: YBasicSeriesSpec[];
nonStacked: YBasicSeriesSpec[];
}
>,
): {
stacked: FormattedDataSeries[];
nonStacked: FormattedDataSeries[];
} {
const specsByGroupIds = splitSpecsByGroupId(specs);
const specsByGroupIdsEntries = [...specsByGroupIds.entries()];

const stackedFormattedDataSeries: {
groupId: GroupId;
dataSeries: DataSeries[];
Expand All @@ -371,7 +375,7 @@ export function getFormattedDataseries(
counts: DataSeriesCounts;
}[] = [];

specsByGroupIdsEntries.forEach(([groupId, groupSpecs]) => {
[...specsByGroupIdsEntries.entries()].forEach(([groupId, groupSpecs]) => {
const { stackMode } = groupSpecs;
// format stacked data series
const stackedDataSeries = getDataSeriesBySpecGroup(groupSpecs.stacked, availableDataSeries);
Expand Down
Loading