fix: render continuous line/area between non-adjacent points (opensea…

…rch-project#833) When rendering line or area charts with multiple series, we generally collect all the available X values and we automatically fill each series with missing x data points (to cover the most used case where we are rendering pre-aggregated data in Kibana). This can cause issues when we want to keep the non-consecutive nature of the line/area. This fix removes the x filling for line and area charts in the following case: non-stacked line or area chart, with continuous scale and no fit function. We apply this fix only to that specific case because: for stacked lines/area we need to clearly compute the stack of each data point on the x-axis, for categorical-ordinal scale (where a line/area chart doesn't always match with the best practice) the x-axis doesn't always describe a continuity within the data. fix opensearch-project#825 BREAKING CHANGE: when rendering non-stacked line/area charts with a continuous x scale and no fit function, the line/area between non-consecutive data points will be rendered as a continuous line/area without adding an uncertain dashed line/ semi-transparent area that connects the two, non-adjacent, points.
AMoo-Miki · Sep 29, 2020 · 5222c40 · 5222c40
1 parent fd8cc2a
commit 5222c40
Show file tree

Hide file tree

Showing 16 changed files with 210 additions and 57 deletions.
diff --git a/packages/osd-charts/.playground/index.html b/packages/osd-charts/.playground/index.html
@@ -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 {
@@ -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>
diff --git a/packages/osd-charts/.playground/playground.tsx b/packages/osd-charts/.playground/playground.tsx
@@ -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>
+  );
 }
diff --git a/...-stories-line-chart-discontinuous-data-points-visually-looks-correct-1-snap.png b/...-stories-line-chart-discontinuous-data-points-visually-looks-correct-1-snap.png
diff --git a/...n-stacked-linear-area-with-discontinuous-data-points-no-fit-function-1-snap.png b/...n-stacked-linear-area-with-discontinuous-data-points-no-fit-function-1-snap.png
diff --git a/...ries-non-stacked-linear-area-with-discontinuous-data-points-with-fit-1-snap.png b/...ries-non-stacked-linear-area-with-discontinuous-data-points-with-fit-1-snap.png
diff --git a/...n-stacked-linear-line-with-discontinuous-data-points-no-fit-function-1-snap.png b/...n-stacked-linear-line-with-discontinuous-data-points-no-fit-function-1-snap.png
diff --git a/...ries-non-stacked-linear-line-with-discontinuous-data-points-with-fit-1-snap.png b/...ries-non-stacked-linear-line-with-discontinuous-data-points-with-fit-1-snap.png
diff --git a/packages/osd-charts/integration/tests/area_stories.test.ts b/packages/osd-charts/integration/tests/area_stories.test.ts
@@ -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',
+      );
+    });
+  });
 });
diff --git a/packages/osd-charts/integration/tests/line_stories.test.ts b/packages/osd-charts/integration/tests/line_stories.test.ts
@@ -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=',
+      );
+    });
+  });
 });
diff --git a/packages/osd-charts/src/chart_types/xy_chart/state/utils/utils.ts b/packages/osd-charts/src/chart_types/xy_chart/state/utils/utils.ts
@@ -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';
@@ -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;
@@ -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);
 
@@ -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);
 
@@ -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
@@ -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);

diff --git a/packages/osd-charts/src/chart_types/xy_chart/utils/fill_series.ts b/packages/osd-charts/src/chart_types/xy_chart/utils/fill_series.ts
@@ -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,
@@ -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;
+  return onlyNoFitAreaLine && onlyContinuous && !isStacked;
+}
diff --git a/packages/osd-charts/src/chart_types/xy_chart/utils/nonstacked_series_utils.test.ts b/packages/osd-charts/src/chart_types/xy_chart/utils/nonstacked_series_utils.test.ts
@@ -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,
@@ -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,
@@ -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,

diff --git a/packages/osd-charts/src/chart_types/xy_chart/utils/series.test.ts b/packages/osd-charts/src/chart_types/xy_chart/utils/series.test.ts
@@ -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,
@@ -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();
   });

diff --git a/packages/osd-charts/src/chart_types/xy_chart/utils/series.ts b/packages/osd-charts/src/chart_types/xy_chart/utils/series.ts
@@ -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';
@@ -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[];
@@ -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);