Addressed code review comments

- Colorway to single var
- Renamed chart theme init

deephaven#1572

packages/chart/src/ChartTheme.module.scss

@@ -5,12 +5,7 @@
   paper-bgcolor: var(--dh-color-chart-bg);
   plot-bgcolor: var(--dh-color-chart-plot-bg);
   title-color: var(--dh-color-chart-title);
-  colorway: var(--dh-color-chart-colorway-1) var(--dh-color-chart-colorway-2)
-    var(--dh-color-chart-colorway-3) var(--dh-color-chart-colorway-4)
-    var(--dh-color-chart-colorway-5) var(--dh-color-chart-colorway-6)
-    var(--dh-color-chart-colorway-7) var(--dh-color-chart-colorway-8)
-    var(--dh-color-chart-colorway-9) var(--dh-color-chart-colorway-10)
-    var(--dh-color-chart-colorway-11);
+  colorway: var(--dh-color-chart-colorway);
   gridcolor: var(--dh-color-chart-grid);
   linecolor: var(--dh-color-chart-axis-line);
   zerolinecolor: var(--dh-color-chart-axis-line-zero);

packages/chart/src/ChartTheme.test.ts

@@ -0,0 +1,36 @@
+/// <reference types="./declaration" />
+
+import { TestUtils } from '@deephaven/utils';
+import { resolveCssVariablesInRecord } from '@deephaven/components';
+import { defaultChartTheme } from './ChartTheme';
+import chartThemeRaw from './ChartTheme.module.scss';
+
+jest.mock('@deephaven/components', () => ({
+  ...jest.requireActual('@deephaven/components'),
+  resolveCssVariablesInRecord: jest.fn(),
+}));
+
+const { asMock } = TestUtils;
+
+const mockChartTheme = new Proxy(
+  {},
+  { get: (_target, name) => `chartTheme['${String(name)}']` }
+);
+
+beforeEach(() => {
+  jest.clearAllMocks();
+  expect.hasAssertions();
+
+  asMock(resolveCssVariablesInRecord)
+    .mockName('resolveCssVariablesInRecord')
+    .mockReturnValue(mockChartTheme);
+});
+
+describe('defaultChartTheme', () => {
+  it('should create the default chart theme', () => {
+    const actual = defaultChartTheme();
+
+    expect(resolveCssVariablesInRecord).toHaveBeenCalledWith(chartThemeRaw);
+    expect(actual).toMatchSnapshot();
+  });
+});

packages/chart/src/ChartTheme.ts

@@ -23,7 +23,7 @@ export interface ChartTheme {
   ohlc_decreasing: string;
 }
 
-export function initChartTheme(): Readonly<ChartTheme> {
+export function defaultChartTheme(): Readonly<ChartTheme> {
   const chartTheme = resolveCssVariablesInRecord(chartThemeRaw);
 
   log.debug2('Chart theme:', chartThemeRaw);
@@ -49,4 +49,4 @@ export function initChartTheme(): Readonly<ChartTheme> {
   });
 }
 
-export default initChartTheme;
+export default defaultChartTheme;

packages/chart/src/ChartThemeProvider.tsx

@@ -1,6 +1,6 @@
 import { createContext, ReactNode, useEffect, useState } from 'react';
 import { useTheme } from '@deephaven/components';
-import initChartTheme, { ChartTheme } from './ChartTheme';
+import defaultChartTheme, { ChartTheme } from './ChartTheme';
 
 export type ChartThemeContextValue = ChartTheme;
 
@@ -26,7 +26,7 @@ export function ChartThemeProvider({
   // the <style> tags to the DOM that provide theme variables
   useEffect(() => {
     if (activeThemes != null) {
-      setChartTheme(initChartTheme());
+      setChartTheme(defaultChartTheme());
     }
   }, [activeThemes]);
 

packages/chart/src/MockChartModel.ts

@@ -4,7 +4,7 @@
 import type { dh as DhType } from '@deephaven/jsapi-types';
 import type { Datum, Layout, PlotData } from 'plotly.js';
 import ChartModel from './ChartModel';
-import { ChartTheme, initChartTheme } from './ChartTheme';
+import { ChartTheme, defaultChartTheme } from './ChartTheme';
 import ChartUtils from './ChartUtils';
 
 interface Series {
@@ -23,7 +23,7 @@ class MockChartModel extends ChartModel {
   static get theme(): ChartTheme {
     /* eslint-disable no-underscore-dangle */
     if (MockChartModel._theme == null) {
-      MockChartModel._theme = initChartTheme();
+      MockChartModel._theme = defaultChartTheme();
     }
 
     return MockChartModel._theme;

packages/chart/src/__snapshots__/ChartTheme.test.ts.snap

@@ -0,0 +1,22 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`defaultChartTheme should create the default chart theme 1`] = `
+{
+  "activecolor": "chartTheme['activecolor']",
+  "area_color": "chartTheme['area-color']",
+  "colorway": "chartTheme['colorway']",
+  "error_band_fill_color": "chartTheme['error-band-fill-color']",
+  "error_band_line_color": "chartTheme['error-band-line-color']",
+  "gridcolor": "chartTheme['gridcolor']",
+  "line_color": "chartTheme['line-color']",
+  "linecolor": "chartTheme['linecolor']",
+  "ohlc_decreasing": "chartTheme['ohlc-decreasing']",
+  "ohlc_increasing": "chartTheme['ohlc-increasing']",
+  "paper_bgcolor": "chartTheme['paper-bgcolor']",
+  "plot_bgcolor": "chartTheme['plot-bgcolor']",
+  "rangebgcolor": "chartTheme['rangebgcolor']",
+  "title_color": "chartTheme['title-color']",
+  "trend_color": "chartTheme['trend-color']",
+  "zerolinecolor": "chartTheme['zerolinecolor']",
+}
+`;

packages/chart/tsconfig.json

@@ -4,13 +4,7 @@
     "rootDir": "src/",
     "outDir": "dist/"
   },
-  "include": [
-    "src/**/*.ts",
-    "src/**/*.tsx",
-    "src/**/*.js",
-    "src/**/*.jsx",
-    "declaration.d.ts"
-  ],
+  "include": ["src/**/*.ts", "src/**/*.tsx", "src/**/*.js", "src/**/*.jsx"],
   "exclude": ["node_modules", "src/**/*.test.*", "src/**/__mocks__/*"],
   "references": [
     {

packages/code-studio/src/styleguide/ThemeColors.tsx

@@ -160,11 +160,32 @@ function extractColorVars(
   return styleText
     .split('\n')
     .map(line => /^\s{2}(--dh-color-(?:[^:]+))/.exec(line)?.[1])
-    .filter(Boolean)
-    .map(varName =>
-      // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-      ({ name: varName!, value: computedStyle.getPropertyValue(varName!)! })
-    );
+    .filter((match): match is string => Boolean(match))
+    .map(varName => {
+      const value = computedStyle.getPropertyValue(varName);
+
+      // Chart colorway consists of multiple colors, so split into separate
+      // swatches for illustration. Note that this assumes the colors are hsl
+      // values. We'll need to make this more robust if we ever change the
+      // default themes to use non-hsl.
+      if (varName === '--dh-color-chart-colorway') {
+        const colorwayColors = value
+          .split('hsl')
+          .filter(Boolean)
+          .map(v => `hsl${v.trim()}`);
+
+        return colorwayColors.map((varExp, i) => ({
+          name: `${varName}-${i}`,
+          value: varExp,
+        }));
+      }
+
+      return {
+        name: varName,
+        value,
+      };
+    })
+    .flat();
 }
 
 /** Group color data based on capture group value */

packages/components/src/theme/theme-dark/theme-dark-semantic-chart.css

@@ -6,18 +6,12 @@
   --dh-color-chart-title: var(--dh-color-white);
 
   /* Colorway */
-  --dh-color-chart-colorway-1: var(--dh-color-visual-cyan);
-  --dh-color-chart-colorway-2: var(--dh-color-visual-green);
-  --dh-color-chart-colorway-3: var(--dh-color-visual-yellow);
-  --dh-color-chart-colorway-4: var(--dh-color-visual-purple);
-  --dh-color-chart-colorway-5: var(--dh-color-visual-orange);
-  --dh-color-chart-colorway-6: var(--dh-color-visual-red);
-  --dh-color-chart-colorway-7: var(--dh-color-visual-chartreuse);
-  --dh-color-chart-colorway-8: var(--dh-color-visual-fuchsia);
-  --dh-color-chart-colorway-9: var(--dh-color-visual-seafoam);
-  --dh-color-chart-colorway-10: var(--dh-color-visual-magenta);
-  --dh-color-chart-colorway-11: var(--dh-color-white);
-
+  --dh-color-chart-colorway: var(--dh-color-visual-cyan)
+    var(--dh-color-visual-green) var(--dh-color-visual-yellow)
+    var(--dh-color-visual-purple) var(--dh-color-visual-orange)
+    var(--dh-color-visual-red) var(--dh-color-visual-chartreuse)
+    var(--dh-color-visual-fuchsia) var(--dh-color-visual-seafoam)
+    var(--dh-color-visual-magenta) var(--dh-color-white);
   --dh-color-chart-grid: var(--dh-color-gray-400);
   --dh-color-chart-axis-line: var(--dh-color-gray-500);
   --dh-color-chart-axis-line-zero: var(--dh-color-gray-700);