fix: Fixes cartesian chart series visibility (WIP) (#158)

* fix: Fixes series visibility in legend on 2nd render

* more tests

Author: pan-kot

pages/01-cartesian-chart/controlled-visibility.page.tsx

@@ -1,6 +1,7 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
+import Checkbox from "@cloudscape-design/components/checkbox";
 import Link from "@cloudscape-design/components/link";
 
 import { CartesianChart, CartesianChartProps } from "../../lib/components";
@@ -9,6 +10,7 @@ import { PageSettings, PageSettingsForm, SeriesFilter, useChartSettings } from "
 import { Page, PageSection } from "../common/templates";
 
 interface ThisPageSettings extends PageSettings {
+  removeHidden: boolean;
   visibleItems: string;
 }
 
@@ -43,6 +45,7 @@ const defaultVisibleItems = "Costs,Costs last year,Peak cost";
 
 export default function () {
   const { settings, setSettings } = useChartSettings<ThisPageSettings>();
+  const removeHidden = settings.removeHidden ?? false;
   const visibleSeries = (settings.visibleItems ?? defaultVisibleItems).split(",");
   return (
     <Page
@@ -53,6 +56,16 @@ export default function () {
         <PageSettingsForm
           selectedSettings={[
             "showLegend",
+            {
+              content: (
+                <Checkbox
+                  checked={removeHidden}
+                  onChange={({ detail }) => setSettings({ removeHidden: detail.checked })}
+                >
+                  Remove hidden
+                </Checkbox>
+              ),
+            },
             {
               content: (
                 <SeriesFilter
@@ -76,12 +89,15 @@ export default function () {
 function ExampleMixedChart() {
   const { settings, setSettings, chartProps } = useChartSettings<ThisPageSettings>();
   const visibleSeries = (settings.visibleItems ?? defaultVisibleItems).split(",");
+  const filteredSeries = settings.removeHidden
+    ? mixedChartSeries.filter((s) => visibleSeries.includes(s.id!))
+    : mixedChartSeries;
   return (
     <CartesianChart
       {...chartProps.cartesian}
       chartHeight={379}
       ariaLabel="Mixed bar chart"
-      series={mixedChartSeries}
+      series={filteredSeries}
       tooltip={{
         point: ({ item }) => {
           return {

pages/02-pie-chart/controlled-visibility.page.tsx

@@ -3,11 +3,14 @@
 
 import { sum } from "lodash";
 
+import Checkbox from "@cloudscape-design/components/checkbox";
+
 import { PieChart, PieChartProps } from "../../lib/components";
 import { PageSettings, PageSettingsForm, SeriesFilter, useChartSettings } from "../common/page-settings";
 import { Page, PageSection } from "../common/templates";
 
 interface ThisPageSettings extends PageSettings {
+  removeHidden: boolean;
   visibleItems: string;
 }
 
@@ -25,6 +28,7 @@ const defaultVisibleItems = "Costs,Costs last year,Peak cost";
 
 export default function () {
   const { settings, setSettings } = useChartSettings<ThisPageSettings>();
+  const removeHidden = settings.removeHidden ?? false;
   const visibleSeries = (settings.visibleItems ?? defaultVisibleItems).split(",");
   return (
     <Page
@@ -35,6 +39,16 @@ export default function () {
         <PageSettingsForm
           selectedSettings={[
             "showLegend",
+            {
+              content: (
+                <Checkbox
+                  checked={removeHidden}
+                  onChange={({ detail }) => setSettings({ removeHidden: detail.checked })}
+                >
+                  Remove hidden
+                </Checkbox>
+              ),
+            },
             {
               content: (
                 <SeriesFilter
@@ -58,12 +72,15 @@ export default function () {
 function ExamplePieChart() {
   const { settings, setSettings, chartProps } = useChartSettings<ThisPageSettings>();
   const visibleSegments = (settings.visibleItems ?? defaultVisibleItems).split(",");
+  const filteredSeries = settings.removeHidden
+    ? { ...pieChartSeries, data: pieChartSeries.data.filter((i) => visibleSegments.includes(i.id!)) }
+    : pieChartSeries;
   return (
     <PieChart
       {...chartProps.pie}
       chartHeight={500}
       ariaLabel="Pie chart"
-      series={pieChartSeries}
+      series={filteredSeries}
       tooltip={{
         details({ segmentValue, totalValue }) {
           return [

src/cartesian-chart/__tests__/cartesian-chart-visibility.test.tsx

@@ -103,4 +103,30 @@ describe("CartesianChart: visibility", () => {
       hiddenSeries: ["L1", "L2"],
     });
   });
+
+  test.each<CartesianChartProps.SeriesOptions>([
+    { type: "area", name: "new", data: [] },
+    { type: "line", name: "new", data: [] },
+    { type: "column", name: "new", data: [] },
+    { type: "scatter", name: "new", data: [] },
+    { type: "x-threshold", name: "new", value: 0 },
+    { type: "y-threshold", name: "new", value: 0 },
+  ])("new series become visible in the legend, type=$type", (newSeries) => {
+    const { rerender } = renderCartesianChart({ ...defaultProps, series: [] });
+    expect(getVisibilityState().allLegendItems).toEqual([]);
+
+    rerender({ highcharts, series: [newSeries] });
+    expect(getVisibilityState().allLegendItems).toEqual(["new"]);
+
+    rerender({ highcharts, series: [newSeries, { ...newSeries, name: "new-2" }] });
+    expect(getVisibilityState().allLegendItems).toEqual(["new", "new-2"]);
+  });
+
+  test("errorbar series do not become visible in the legend", () => {
+    const { rerender } = renderCartesianChart({ ...defaultProps, series: lineSeries });
+    expect(getVisibilityState().allLegendItems).toEqual(["L1", "L2"]);
+
+    rerender({ highcharts, series: [...lineSeries, { type: "errorbar", name: "EB", linkedTo: "L2", data: [] }] });
+    expect(getVisibilityState().allLegendItems).toEqual(["L1", "L2"]);
+  });
 });

src/cartesian-chart/chart-series-cartesian.tsx

@@ -37,6 +37,10 @@ export const transformCartesianSeries = (
   const thresholdX = getThresholdX(originalSeries, visibleSeries);
   const thresholdY = getThresholdY(originalSeries, visibleSeries);
   function transformSeriesToHighcharts(s: CartesianChartProps.SeriesOptions): Highcharts.SeriesOptionsType {
+    // Highcharts does not properly update data on re-render. When the fake empty series is replaced with the real series,
+    // the properties are merged together, so the showInLegend=false is not overridden.
+    // That is why we explicitly override this property for all real series.
+    const shared = { showInLegend: true };
     if (s.type === "x-threshold" || s.type === "y-threshold") {
       const data =
         s.type === "x-threshold"
@@ -49,7 +53,7 @@ export const transformCartesianSeries = (
         color: s.color ?? Styles.thresholdSeries.color,
         dashStyle: s.dashStyle ?? Styles.thresholdSeries.dashStyle,
       };
-      return { type: "line", id: s.id, name: s.name, data, custom, enableMouseTracking, ...style };
+      return { type: "line", id: s.id, name: s.name, data, custom, enableMouseTracking, ...style, ...shared };
     }
     if (s.type === "errorbar") {
       const color = s.color ?? colorChartsErrorBarMarker;
@@ -58,7 +62,7 @@ export const transformCartesianSeries = (
       const colors = { stemColor: color, whiskerColor: color };
       return { ...s, data: s.data as Writeable<RangeDataItemOptions[]>, ...colors };
     }
-    return { ...s, data: s.data as Writeable<PointDataItemType[]> };
+    return { ...s, data: s.data as Writeable<PointDataItemType[]>, ...shared };
   }
   const series = originalSeries.map(transformSeriesToHighcharts);
   // We inject a fake empty series so that the empty state still shows axes, if defined.

src/core/__tests__/chart-core-legend.test.tsx

@@ -49,7 +49,6 @@ const series: Highcharts.SeriesOptionsType[] = [
       { name: "P2", y: 30 },
       { id: "P3", name: "Pie 3", y: 60 },
     ],
-    showInLegend: true,
   },
 ];
 
@@ -116,6 +115,14 @@ describe("CoreChart: legend", () => {
     expect(getItems({ active: false }).map((w) => w.getElement().textContent)).toEqual(["L2", "Line 3", "P2", "Pie 3"]);
   });
 
+  test("does not render series with showInLegend=false", () => {
+    const sVisible = series.filter((s) => s.name === "L1" || s.name === "L2");
+    const sHidden = series.filter((s) => !sVisible.includes(s)).map((s) => ({ ...s, showInLegend: false }));
+
+    renderChart({ highcharts, options: { series: [...sVisible, ...sHidden] } });
+    expect(getItems().map((w) => w.getElement().textContent)).toEqual(["L1", "L2", "P1", "P2", "Pie 3"]);
+  });
+
   test("does not render title by default", () => {
     renderChart({ highcharts, options: { series: series } });
 

src/core/__tests__/chart-core-visibility.test.tsx

@@ -52,7 +52,6 @@ const pieSeries: Highcharts.SeriesOptionsType[] = [
       { name: "B", y: 20 },
       { name: "C", y: 70 },
     ],
-    showInLegend: true,
   },
 ];
 
@@ -377,7 +376,6 @@ describe("CoreChart: visibility", () => {
           { id: "1", name: "Segment", y: 20 },
           { id: "2", name: "Segment", y: 80 },
         ],
-        showInLegend: true,
       },
     ];
     const { rerender } = renderChart({