feat: Chart responsible for its own theme

packages/chart/src/Chart.tsx

@@ -29,12 +29,15 @@ import {
   ModeBarButtonAny,
 } from 'plotly.js';
 import type { PlotParams } from 'react-plotly.js';
+import { bindAllMethods } from '@deephaven/utils';
 import createPlotlyComponent from './plotly/createPlotlyComponent';
 import Plotly from './plotly/Plotly';
 import ChartModel from './ChartModel';
+import { ChartTheme } from './ChartTheme';
 import ChartUtils, { ChartModelSettings } from './ChartUtils';
 import './Chart.scss';
 import DownsamplingError from './DownsamplingError';
+import useChartTheme from './useChartTheme';
 
 const log = Log.module('Chart');
 
@@ -46,6 +49,7 @@ type FormatterSettings = ColumnFormatSettings &
 
 interface ChartProps {
   model: ChartModel;
+  theme: ChartTheme;
   settings: FormatterSettings;
   isActive: boolean;
   Plotly: typeof Plotly;
@@ -57,6 +61,12 @@ interface ChartProps {
   onSettingsChanged: (settings: Partial<ChartModelSettings>) => void;
 }
 
+// All of the ChartProps have default values except for model in the Chart
+// component, hence the Partial here.
+interface ChartContainerProps extends Partial<Omit<ChartProps, 'theme'>> {
+  model: ChartModel;
+}
+
 interface ChartState {
   data: Partial<Data>[] | null;
   /** An error specific to downsampling */
@@ -72,7 +82,7 @@ interface ChartState {
   revision: number;
 }
 
-export class Chart extends Component<ChartProps, ChartState> {
+class Chart extends Component<ChartProps, ChartState> {
   static defaultProps = {
     isActive: true,
     settings: {
@@ -135,14 +145,7 @@ export class Chart extends Component<ChartProps, ChartState> {
   constructor(props: ChartProps) {
     super(props);
 
-    this.handleAfterPlot = this.handleAfterPlot.bind(this);
-    this.handleDownsampleClick = this.handleDownsampleClick.bind(this);
-    this.handleErrorClose = this.handleErrorClose.bind(this);
-    this.handleModelEvent = this.handleModelEvent.bind(this);
-    this.handlePlotUpdate = this.handlePlotUpdate.bind(this);
-    this.handleRelayout = this.handleRelayout.bind(this);
-    this.handleResize = this.handleResize.bind(this);
-    this.handleRestyle = this.handleRestyle.bind(this);
+    bindAllMethods(this);
 
     this.PlotComponent = createPlotlyComponent(props.Plotly);
     this.plot = React.createRef();
@@ -186,10 +189,12 @@ export class Chart extends Component<ChartProps, ChartState> {
     if (this.plotWrapper.current != null) {
       this.resizeObserver.observe(this.plotWrapper.current);
     }
+
+    this.handleThemeChange();
   }
 
   componentDidUpdate(prevProps: ChartProps): void {
-    const { isActive, model, settings } = this.props;
+    const { isActive, model, settings, theme } = this.props;
     this.updateFormatterSettings(settings as FormatterSettings);
 
     if (model !== prevProps.model) {
@@ -205,6 +210,10 @@ export class Chart extends Component<ChartProps, ChartState> {
         this.unsubscribe(model);
       }
     }
+
+    if (theme !== prevProps.theme) {
+      this.handleThemeChange();
+    }
   }
 
   componentWillUnmount(): void {
@@ -348,14 +357,14 @@ export class Chart extends Component<ChartProps, ChartState> {
 
   initData(): void {
     const { model } = this.props;
-    const { layout } = this.state;
-    this.setState({
+
+    this.setState(({ layout }) => ({
       data: model.getData(),
       layout: {
         ...layout,
         ...model.getLayout(),
       },
-    });
+    }));
   }
 
   subscribe(model: ChartModel): void {
@@ -539,6 +548,19 @@ export class Chart extends Component<ChartProps, ChartState> {
     }
   }
 
+  handleThemeChange(): void {
+    const { theme, model } = this.props;
+    const { dh } = model;
+    const chartUtils = new ChartUtils(dh);
+
+    this.setState(({ layout }) => ({
+      layout: {
+        ...layout,
+        template: chartUtils.makeDefaultTemplate(theme),
+      },
+    }));
+  }
+
   /**
    * Toggle the error message. If it is already being displayed, then hide it.
    */
@@ -661,6 +683,7 @@ export class Chart extends Component<ChartProps, ChartState> {
       error
     );
     const isPlotShown = data != null;
+
     return (
       <div className="h-100 w-100 chart-wrapper" ref={this.plotWrapper}>
         {isPlotShown && (
@@ -703,4 +726,10 @@ export class Chart extends Component<ChartProps, ChartState> {
   }
 }
 
-export default Chart;
+export default function ChartContainer(
+  props: ChartContainerProps
+): JSX.Element {
+  const chartTheme = useChartTheme();
+  // eslint-disable-next-line react/jsx-props-no-spreading
+  return <Chart {...props} theme={chartTheme} />;
+}

packages/chart/src/ChartModelFactory.ts

@@ -1,7 +1,6 @@
 import type { dh as DhType, Figure, Table } from '@deephaven/jsapi-types';
 import ChartUtils, { ChartModelSettings } from './ChartUtils';
 import FigureChartModel from './FigureChartModel';
-import { ChartTheme } from './ChartTheme';
 import ChartModel from './ChartModel';
 
 class ChartModelFactory {
@@ -16,23 +15,21 @@ class ChartModelFactory {
    * @param settings.xAxis The column name to use for the x-axis
    * @param [settings.hiddenSeries] Array of hidden series names
    * @param table The table to build the model for
-   * @param theme The theme for the figure
    * @returns The ChartModel Promise representing the figure
    * CRA sets tsconfig to type check JS based on jsdoc comments. It isn't able to figure out FigureChartModel extends ChartModel
    * This causes TS issues in 1 or 2 spots. Once this is TS it can be returned to just FigureChartModel
    */
   static async makeModelFromSettings(
     dh: DhType,
     settings: ChartModelSettings,
-    table: Table,
-    theme: ChartTheme
+    table: Table
   ): Promise<ChartModel> {
     const figure = await ChartModelFactory.makeFigureFromSettings(
       dh,
       settings,
       table
     );
-    return new FigureChartModel(dh, figure, theme, settings);
+    return new FigureChartModel(dh, figure, settings);
   }
 
   /**
@@ -78,18 +75,16 @@ class ChartModelFactory {
    * @param settings.xAxis The column name to use for the x-axis
    * @param [settings.hiddenSeries] Array of hidden series names
    * @param figure The figure to build the model for
-   * @param theme The theme for the figure
    * @returns The FigureChartModel representing the figure
    * CRA sets tsconfig to type check JS based on jsdoc comments. It isn't able to figure out FigureChartModel extends ChartModel
    * This causes TS issues in 1 or 2 spots. Once this is TS it can be returned to just FigureChartModel
    */
   static async makeModel(
     dh: DhType,
     settings: ChartModelSettings | undefined,
-    figure: Figure,
-    theme: ChartTheme
+    figure: Figure
   ): Promise<ChartModel> {
-    return new FigureChartModel(dh, figure, theme, settings);
+    return new FigureChartModel(dh, figure, settings);
   }
 }
 

packages/chart/src/ChartTestUtils.ts

@@ -8,6 +8,7 @@ import type {
   Figure,
   Series,
   SeriesDataSource,
+  SeriesPlotStyle,
 } from '@deephaven/jsapi-types';
 
 class ChartTestUtils {
@@ -84,6 +85,12 @@ class ChartTestUtils {
     sources = this.makeDefaultSources(),
     lineColor = null,
     shapeColor = null,
+  }: {
+    name?: string | null;
+    lineColor?: string | null;
+    plotStyle?: SeriesPlotStyle | null;
+    shapeColor?: string | null;
+    sources?: SeriesDataSource[];
   } = {}): Series {
     const { dh } = this;
     // eslint-disable-next-line @typescript-eslint/no-explicit-any

packages/chart/src/ChartUtils.test.ts

@@ -1,6 +1,5 @@
 import dh from '@deephaven/jsapi-shim';
 import { Formatter } from '@deephaven/jsapi-utils';
-import { TestUtils } from '@deephaven/utils';
 import { Layout } from 'plotly.js';
 import ChartUtils from './ChartUtils';
 import ChartTestUtils from './ChartTestUtils';
@@ -9,16 +8,29 @@ import type { ChartTheme } from './ChartTheme';
 const chartUtils = new ChartUtils(dh);
 const chartTestUtils = new ChartTestUtils(dh);
 
-const { createMockProxy } = TestUtils;
-
 function makeFormatter() {
   return new Formatter(dh);
 }
 
-const chartTheme = createMockProxy<ChartTheme>();
+// Proxy for ChartTheme
+const chartTheme = new Proxy(
+  {},
+  {
+    get(_target, name) {
+      if (name === 'colorway') {
+        return 'ChartTheme[colorway0] ChartTheme[colorway1] ChartTheme[colorway2]';
+      }
+
+      return `ChartTheme['${String(name)}']`;
+    },
+  }
+) as ChartTheme;
 
 it('groups the axes by type properly', () => {
-  const testAxes = (axes, expectedResult) => {
+  const testAxes = (
+    axes: { type: string; label: string }[],
+    expectedResult
+  ) => {
     const chart = { axes };
     const result = ChartUtils.groupArray(chart.axes, 'type');
     expect(result).toEqual(expectedResult);
@@ -199,7 +211,7 @@ describe('updating layout axes', () => {
   it('adds new axes', () => {
     const layout = {};
     const axes = makeTwinAxes();
-    chartUtils.updateLayoutAxes(layout, axes, axes, chartTheme);
+    chartUtils.updateLayoutAxes(layout, axes, axes);
     expect(layout).toEqual(
       expect.objectContaining({
         xaxis: expect.objectContaining({
@@ -221,22 +233,26 @@ describe('updating layout axes', () => {
   it('keeps the same axis objects, updates domain', () => {
     const layout: Partial<Layout> = {};
     const axes = makeTwinAxes();
-    chartUtils.updateLayoutAxes(layout, axes, axes, chartTheme, 10);
+    chartUtils.updateLayoutAxes(layout, axes, axes, 10);
 
     const { xaxis } = layout;
     const xDomain = [...(xaxis?.domain ?? [])];
-    chartUtils.updateLayoutAxes(layout, axes, axes, chartTheme, 1000);
+    chartUtils.updateLayoutAxes(layout, axes, axes, 1000);
 
     expect(layout.xaxis).toBe(xaxis);
     expect(xDomain).not.toBe(xaxis?.domain);
   });
 
   it('removes stale axes', () => {
-    const layout = {};
+    const layout = {
+      xaxis: {},
+      yaxis: {},
+      yaxis2: {},
+    };
     const axes = makeTwinAxes();
     const chart = chartTestUtils.makeChart({ axes });
     const figure = chartTestUtils.makeFigure({ charts: [chart] });
-    chartUtils.updateFigureAxes(layout, figure, chartTheme);
+    chartUtils.updateFigureAxes(layout, figure);
     expect(layout).toEqual(
       expect.objectContaining({
         xaxis: expect.objectContaining({}),
@@ -246,7 +262,7 @@ describe('updating layout axes', () => {
     );
 
     axes.pop();
-    chartUtils.updateFigureAxes(layout, figure, chartTheme);
+    chartUtils.updateFigureAxes(layout, figure);
     expect(layout).toEqual(
       expect.objectContaining({
         xaxis: expect.objectContaining({}),
@@ -305,7 +321,6 @@ describe('updating layout axes', () => {
         layout,
         axes,
         figureAxes,
-        chartTheme,
         width,
         height,
         bounds
@@ -646,3 +661,10 @@ describe('getMarkerSymbol', () => {
     expect(() => getMarkerSymbol('&$*(#@&')).toThrow();
   });
 });
+
+describe('makeDefaultTemplate', () => {
+  it('should create a default template', () => {
+    const template = chartUtils.makeDefaultTemplate(chartTheme);
+    expect(template).toMatchSnapshot();
+  });
+});