feat: Chart responsible for its own theme (#1772)

This PR 
- Move theme colors down to `layout.template.layout` so that they are
used as defaults. Some of the theme colors had to be set in
`layout.template.data` for specific trace types
- Chart component now responds to ChartTheme context changes (this
should enable chart theme switching, but it depends on some follow up
plugins repo work to complete it).

**Testing**
For now, we should just make sure charts behave as they did before. Not
all charts will update in response to a theme change, but re-running
queries should show the theme change. I have tested this and have not
seen any changes to existing behavior introduced by this PR

Once the following plugins repo PRs have merged, the chart should fully
respond to theme changes:
deephaven/deephaven-plugins#243
deephaven/deephaven-plugins#251

resolves #1728

BREAKING CHANGE: 
- Renamed `ColorUtils.getColorwayFromTheme` to `normalizeColorway`
- Removed `chartTheme` arg from functions in `ChartUtils`,
`ChartModelFactory` and `FigureChartModel` in @deephaven/chart

Author: bmingles

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();
+  });
+});