fix: point style accessor for isolated points (#2464)

elastic · Jun 12, 2024 · ee7f529 · ee7f529
1 parent 501781b
commit ee7f529
Show file tree

Hide file tree

Showing 12 changed files with 176 additions and 15 deletions.
diff --git a/...l.test.ts-snapshots/baselines/test-cases/point-style-overrides-chrome-linux.png b/...l.test.ts-snapshots/baselines/test-cases/point-style-overrides-chrome-linux.png
diff --git a/...es/should-override-all-point-styles-with-point-accessor-styles-chrome-linux.png b/...es/should-override-all-point-styles-with-point-accessor-styles-chrome-linux.png
diff --git a/...d-override-theme-and-series-point-styles-with-iso-point-styles-chrome-linux.png b/...d-override-theme-and-series-point-styles-with-iso-point-styles-chrome-linux.png
diff --git a/...d-override-theme-point-and-iso-point-styles-with-series-styles-chrome-linux.png b/...d-override-theme-point-and-iso-point-styles-with-series-styles-chrome-linux.png
diff --git a/e2e/tests/test_cases_stories.test.ts b/e2e/tests/test_cases_stories.test.ts
@@ -146,4 +146,24 @@ test.describe('Test cases stories', () => {
       );
     });
   });
+
+  test.describe('Isolated point styles', () => {
+    test('should override theme point and iso point styles with series styles', async ({ page }) => {
+      await common.expectChartAtUrlToMatchScreenshot(page)(
+        'http://localhost:9001/?path=/story/test-cases--point-style-overrides&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-isolatedPoint.stroke - series level=orange&knob-isolatedPoint.stroke - theme level=green&knob-point.stroke - series level=blue&knob-point.stroke - theme level=red&knob-series type=line&knob-stroke - pointStyleAccessor=black&knob-use point style overrides=false&knob-use series iso overrides=false&knob-use series overrides=true',
+      );
+    });
+
+    test('should override theme and series point styles with iso point styles', async ({ page }) => {
+      await common.expectChartAtUrlToMatchScreenshot(page)(
+        'http://localhost:9001/?path=/story/test-cases--point-style-overrides&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-isolatedPoint.stroke - series level=orange&knob-isolatedPoint.stroke - theme level=green&knob-point.stroke - series level=blue&knob-point.stroke - theme level=red&knob-series type=line&knob-stroke - pointStyleAccessor=black&knob-use point style overrides=false&knob-use series iso overrides=true&knob-use series overrides=true',
+      );
+    });
+
+    test('should override all point styles with point accessor styles', async ({ page }) => {
+      await common.expectChartAtUrlToMatchScreenshot(page)(
+        'http://localhost:9001/?path=/story/test-cases--point-style-overrides&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-isolatedPoint.stroke - series level=orange&knob-isolatedPoint.stroke - theme level=green&knob-point.stroke - series level=blue&knob-point.stroke - theme level=red&knob-series type=line&knob-stroke - pointStyleAccessor=black&knob-use point style overrides=true&knob-use series iso overrides=true&knob-use series overrides=true',
+      );
+    });
+  });
 });
diff --git a/packages/charts/api/charts.api.md b/packages/charts/api/charts.api.md
@@ -2446,7 +2446,7 @@ export interface PointStyle {
 }
 
 // @public
-export type PointStyleAccessor = (datum: DataSeriesDatum, seriesIdentifier: XYChartSeriesIdentifier) => PointStyleOverride;
+export type PointStyleAccessor = (datum: DataSeriesDatum, seriesIdentifier: XYChartSeriesIdentifier, isolatedPoint: boolean) => PointStyleOverride;
 
 // @public (undocumented)
 export type PointStyleOverride = RecursivePartial<PointStyle> | Color | null;

diff --git a/packages/charts/src/chart_types/xy_chart/rendering/points.ts b/packages/charts/src/chart_types/xy_chart/rendering/points.ts
@@ -79,10 +79,10 @@ export function renderPoints(
       const y = yDatumKeyName === 'y1' ? y1Fn(datum) : y0Fn(datum);
       const originalY = getDatumYValue(datum, keyIndex === 0, isBandedSpec, dataSeries.stackMode);
       const seriesIdentifier: XYChartSeriesIdentifier = getSeriesIdentifierFromDataSeries(dataSeries);
-      const styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, styleAccessor);
-      const style = buildPointGeometryStyles(color, pointStyle, styleOverrides);
       const isPointIsolated = allowIsolated && isIsolatedPoint(dataIndex, dataSeries.data.length, yDefined, prev, next);
-      const isolatedPointStyle = buildPointGeometryStyles(color, isolatedPointThemeStyle);
+      const styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, isPointIsolated, styleAccessor);
+      const style = buildPointGeometryStyles(color, pointStyle, styleOverrides);
+      const isolatedPointStyle = buildPointGeometryStyles(color, isolatedPointThemeStyle, styleOverrides);
       // if radius is defined with the mark, limit the minimum radius to the theme radius value
       const radius = isPointIsolated
         ? isolatedPointThemeStyle.radius
@@ -134,9 +134,10 @@ export function renderPoints(
 export function getPointStyleOverrides(
   datum: DataSeriesDatum,
   seriesIdentifier: XYChartSeriesIdentifier,
+  isolatedPoint: boolean,
   pointStyleAccessor?: PointStyleAccessor,
 ): Partial<PointStyle> | undefined {
-  const styleOverride = pointStyleAccessor && pointStyleAccessor(datum, seriesIdentifier);
+  const styleOverride = pointStyleAccessor && pointStyleAccessor(datum, seriesIdentifier, isolatedPoint);
 
   if (!styleOverride) {
     return;

diff --git a/packages/charts/src/chart_types/xy_chart/rendering/rendering.test.ts b/packages/charts/src/chart_types/xy_chart/rendering/rendering.test.ts
@@ -323,28 +323,28 @@ describe('Rendering utils', () => {
     });
 
     it('should return undefined if no pointStyleAccessor is passed', () => {
-      const styleOverrides = getPointStyleOverrides(datum, seriesIdentifier);
+      const styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, false);
 
       expect(styleOverrides).toBeUndefined();
     });
 
     it('should return undefined if pointStyleAccessor returns null', () => {
       mockAccessor.mockReturnValue(null);
-      const styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, mockAccessor);
+      const styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, false, mockAccessor);
 
       expect(styleOverrides).toBeUndefined();
     });
 
-    it('should call pointStyleAccessor with datum and seriesIdentifier', () => {
-      getPointStyleOverrides(datum, seriesIdentifier, mockAccessor);
+    it('should call pointStyleAccessor with datum, seriesIdentifier and isolatedPoint', () => {
+      getPointStyleOverrides(datum, seriesIdentifier, true, mockAccessor);
 
-      expect(mockAccessor).toHaveBeenCalledWith(datum, seriesIdentifier);
+      expect(mockAccessor).toHaveBeenCalledWith(datum, seriesIdentifier, true);
     });
 
     it('should return seriesStyle with updated stroke color', () => {
       const stroke = 'blue';
       mockAccessor.mockReturnValue(stroke);
-      const styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, mockAccessor);
+      const styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, false, mockAccessor);
       const expectedStyles: Partial<PointStyle> = {
         stroke,
       };

diff --git a/packages/charts/src/chart_types/xy_chart/utils/specs.ts b/packages/charts/src/chart_types/xy_chart/utils/specs.ts
@@ -101,6 +101,7 @@ export type BarStyleAccessor = (datum: DataSeriesDatum, seriesIdentifier: XYChar
 export type PointStyleAccessor = (
   datum: DataSeriesDatum,
   seriesIdentifier: XYChartSeriesIdentifier,
+  isolatedPoint: boolean,
 ) => PointStyleOverride;
 
 /**

diff --git a/storybook/stories/line/12_isolated_data_points.story.tsx b/storybook/stories/line/12_isolated_data_points.story.tsx
@@ -31,19 +31,19 @@ export const Example: ChartsStory = (_, { title, description }) => {
     max: 10,
     step: 1,
   });
-  const radius = number('isolated point radius', 2, {
+  const overrideRadius = number('override point radius', 0, {
     range: true,
     min: 0,
     max: 10,
     step: 1,
   });
-  const overrideRadius = number('override point radius', 0, {
+  const overridePointRadius = boolean('override radius for isolated points', false);
+  const radius = number('isolated point radius', 2, {
     range: true,
     min: 0,
     max: 10,
     step: 1,
   });
-  const overridePointRadius = boolean('override radius for isolated points', false);
 
   return (
     <Chart title={title} description={description}>
@@ -77,7 +77,7 @@ export const Example: ChartsStory = (_, { title, description }) => {
         id="x"
         position={Position.Bottom}
         style={{
-          tickLine: { size: 0.0001, padding: 4 },
+          tickLine: { size: 0, padding: 4 },
           tickLabel: {
             alignment: { horizontal: Position.Left, vertical: Position.Bottom },
             padding: 0,

diff --git a/storybook/stories/test_cases/13_point_style_overrides.story.tsx b/storybook/stories/test_cases/13_point_style_overrides.story.tsx
@@ -0,0 +1,138 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { boolean, color } from '@storybook/addon-knobs';
+import React from 'react';
+
+import { Axis, Chart, Position, ScaleType, Settings } from '@elastic/charts';
+import { KIBANA_METRICS } from '@elastic/charts/src/utils/data_samples/test_dataset_kibana';
+
+import { ChartsStory } from '../../types';
+import { useBaseTheme } from '../../use_base_theme';
+import { customKnobs } from '../utils/knobs';
+
+const defaultPointStyles = {
+  radius: 4,
+  strokeWidth: 2,
+};
+
+const defaultlineStyles = {
+  stroke: 'black',
+};
+
+export const Example: ChartsStory = (_, { title, description }) => {
+  const [Series] = customKnobs.enum.xySeries('series type', 'line', { exclude: ['bar', 'bubble'] });
+  const themeLevelStroke = color('point.stroke - theme level', 'red');
+  const themeLevelStrokeIso = color('isolatedPoint.stroke - theme level', 'green');
+  const useSeriesOverrides = boolean('use series overrides', false);
+  const seriesLevelStroke = color('point.stroke - series level', 'blue');
+  const useSeriesIsoOverrides = boolean('use series iso overrides', false);
+  const seriesLevelStrokeIso = color('isolatedPoint.stroke - series level', 'orange');
+  const usePointStyleOverrides = boolean('use point style overrides', false);
+  const pointStyleStroke = color('stroke - pointStyleAccessor', 'black');
+
+  return (
+    <Chart title={title} description={description}>
+      <Settings
+        legendPosition={Position.Right}
+        theme={{
+          areaSeriesStyle: {
+            line: defaultlineStyles,
+            point: {
+              visible: true,
+              stroke: themeLevelStroke,
+              ...defaultPointStyles,
+            },
+            isolatedPoint: {
+              stroke: themeLevelStrokeIso,
+              ...defaultPointStyles,
+            },
+          },
+          lineSeriesStyle: {
+            line: defaultlineStyles,
+            point: {
+              visible: true,
+              stroke: themeLevelStroke,
+              ...defaultPointStyles,
+            },
+            isolatedPoint: {
+              stroke: themeLevelStrokeIso,
+              ...defaultPointStyles,
+            },
+          },
+        }}
+        baseTheme={useBaseTheme()}
+      />
+      <Axis
+        id="x"
+        position={Position.Bottom}
+        style={{
+          tickLine: { size: 0, padding: 4 },
+          tickLabel: {
+            alignment: { horizontal: Position.Left, vertical: Position.Bottom },
+            padding: 0,
+            offset: { x: 0, y: 0 },
+          },
+        }}
+        timeAxisLayerCount={2}
+        gridLine={{
+          visible: true,
+        }}
+      />
+      <Axis id="y" position={Position.Left} ticks={5} />
+
+      <Series
+        id="Count of records"
+        xScaleType={ScaleType.Time}
+        yScaleType={ScaleType.Linear}
+        xAccessor={0}
+        yAccessors={[1]}
+        splitSeriesAccessors={[2]}
+        pointStyleAccessor={
+          !usePointStyleOverrides
+            ? undefined
+            : () => ({
+                stroke: pointStyleStroke,
+              })
+        }
+        areaSeriesStyle={{
+          ...(useSeriesOverrides && {
+            point: {
+              stroke: seriesLevelStroke,
+            },
+          }),
+          ...(useSeriesIsoOverrides && {
+            isolatedPoint: {
+              stroke: seriesLevelStrokeIso,
+            },
+          }),
+        }}
+        lineSeriesStyle={{
+          ...(useSeriesOverrides && {
+            point: {
+              stroke: seriesLevelStroke,
+            },
+          }),
+          ...(useSeriesIsoOverrides && {
+            isolatedPoint: {
+              stroke: seriesLevelStrokeIso,
+            },
+          }),
+        }}
+        data={[
+          ...KIBANA_METRICS.metrics.kibana_os_load.v1.data.slice(0, 40).map((d, i) => {
+            if ([1, 10, 12, 20, 22, 24, 28].includes(i)) {
+              return [d[0], null, 'A'];
+            }
+            return [...d, 'A'];
+          }),
+        ]}
+      />
+    </Chart>
+  );
+};
diff --git a/storybook/stories/test_cases/test_cases.stories.tsx b/storybook/stories/test_cases/test_cases.stories.tsx
@@ -24,3 +24,4 @@ export { Example as domainEdges } from './21_domain_edges.story';
 export { Example as metricTrendLength } from './22_metric_trend_length.story';
 export { Example as startDayOfWeek } from './11_start_day_of_week.story';
 export { Example as logWithNegativeValues } from './12_log_with_negative_values.story';
+export { Example as pointStyleOverrides } from './13_point_style_overrides.story';