feat: Remove UI theme from PlotlyExpressChartModel

Author: mattrunyon

plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts

@@ -7,35 +7,24 @@ import type {
   TableSubscription,
   TableData,
 } from '@deephaven/jsapi-types';
-import {
-  ChartModel,
-  ChartUtils,
-  ChartTheme,
-  defaultChartTheme,
-} from '@deephaven/chart';
+import { ChartModel, ChartUtils } from '@deephaven/chart';
 import Log from '@deephaven/log';
 import {
   PlotlyChartWidgetData,
-  applyColorwayToData,
   getDataMappings,
   getWidgetData,
+  removeColorsFromData,
 } from './PlotlyExpressChartUtils.js';
 
 const log = Log.module('@deephaven/js-plugin-plotly-express.ChartModel');
 
 export class PlotlyExpressChartModel extends ChartModel {
-  constructor(
-    dh: DhType,
-    widget: Widget,
-    refetch: () => Promise<Widget>,
-    theme: ChartTheme = defaultChartTheme()
-  ) {
+  constructor(dh: DhType, widget: Widget, refetch: () => Promise<Widget>) {
     super(dh);
 
     this.widget = widget;
     this.refetch = refetch;
     this.chartUtils = new ChartUtils(dh);
-    this.theme = theme;
 
     this.handleFigureUpdated = this.handleFigureUpdated.bind(this);
     this.handleWidgetUpdated = this.handleWidgetUpdated.bind(this);
@@ -89,14 +78,10 @@ export class PlotlyExpressChartModel extends ChartModel {
    */
   tableDataMap: Map<number, { [key: string]: unknown[] }> = new Map();
 
-  theme: ChartTheme;
-
   plotlyData: Data[] = [];
 
   layout: Partial<Layout> = {};
 
-  plotlyLayout: Partial<Layout> = {};
-
   isPaused = false;
 
   hasPendingUpdate = false;
@@ -197,22 +182,11 @@ export class PlotlyExpressChartModel extends ChartModel {
 
   updateLayout(data: PlotlyChartWidgetData) {
     const { figure } = data;
-    const { plotly, deephaven } = figure;
+    const { plotly } = figure;
     const { layout: plotlyLayout = {} } = plotly;
 
-    const template = { layout: this.chartUtils.makeDefaultLayout(this.theme) };
-
-    // For now we will only use the plotly theme colorway since most plotly themes are light mode
-    if (deephaven.is_user_set_template) {
-      template.layout.colorway =
-        plotlyLayout.template?.layout?.colorway ?? template.layout.colorway;
-    }
-
-    this.plotlyLayout = plotlyLayout;
-
     this.layout = {
       ...plotlyLayout,
-      template,
     };
   }
 
@@ -225,17 +199,19 @@ export class PlotlyExpressChartModel extends ChartModel {
       new_references: newReferences,
       removed_references: removedReferences,
     } = data;
-    const { plotly } = figure;
+    const { plotly, deephaven } = figure;
+    const { layout: plotlyLayout = {} } = plotly;
     this.tableColumnReplacementMap = getDataMappings(data);
 
     this.plotlyData = plotly.data;
     this.updateLayout(data);
 
-    applyColorwayToData(
-      this.layout?.template?.layout?.colorway ?? [],
-      this.plotlyLayout?.template?.layout?.colorway ?? [],
-      this.plotlyData
-    );
+    if (!deephaven.is_user_set_template) {
+      removeColorsFromData(
+        plotlyLayout?.template?.layout?.colorway ?? [],
+        this.plotlyData
+      );
+    }
 
     newReferences.forEach(async (id, i) => {
       this.tableDataMap.set(id, {}); // Plot may render while tables are being fetched. Set this to avoid a render error

plugins/plotly-express/src/js/src/PlotlyExpressChartUtils.ts

@@ -1,8 +1,5 @@
 import type { Data, PlotlyDataLayoutConfig } from 'plotly.js';
 import type { Widget } from '@deephaven/jsapi-types';
-import Log from '@deephaven/log';
-
-const log = Log.module('@deephaven/js-plugin-plotly-express.ChartUtils');
 
 export interface PlotlyChartWidgetData {
   type: string;
@@ -57,39 +54,20 @@ export function getDataMappings(
 }
 
 /**
- * Applies the colorway to the data unless the data color is not its default value
- * Data color is not default if the user set the color specifically or the plot type sets it
+ * Removes the default colors from the data
+ * Data color is not removed if the user set the color specifically or the plot type sets it
+ *
+ * This only checks if the marker or line color is set to a color in the colorway.
+ * This means it is not possible to change the order of the colorway and use the same colors.
  *
- * @param colorway The colorway from the web UI
- * @param plotlyColorway The colorway from plotly
- * @param data The data to apply the colorway to. This will be mutated
+ * @param colorway The colorway from plotly
+ * @param data The data to remove the colorway from. This will be mutated
  */
-export function applyColorwayToData(
-  colorway: string[],
-  plotlyColorway: string[],
-  data: Data[]
-): void {
-  if (colorway.length === 0) {
-    return;
-  }
-
-  if (plotlyColorway.length > colorway.length) {
-    log.warn(
-      "Plotly's colorway is longer than the web UI colorway. May result in incorrect colors for some series"
-    );
-  }
-
-  const colorMap = new Map(
-    plotlyColorway.map((color, i) => [
-      color.toUpperCase(),
-      colorway[i] ?? color,
-    ])
-  );
-
-  const plotlyColors = new Set(
-    plotlyColorway.map(color => color.toUpperCase())
-  );
+export function removeColorsFromData(colorway: string[], data: Data[]): void {
+  const plotlyColors = new Set(colorway.map(color => color.toUpperCase()));
 
+  // Just check if the colors are in the colorway at any point
+  // Plotly has many different ways to layer/order series
   for (let i = 0; i < data.length; i += 1) {
     const trace = data[i];
 
@@ -101,7 +79,7 @@ export function applyColorwayToData(
       typeof trace.marker.color === 'string'
     ) {
       if (plotlyColors.has(trace.marker.color.toUpperCase())) {
-        trace.marker.color = colorMap.get(trace.marker.color.toUpperCase());
+        delete trace.marker.color;
       }
     }
 
@@ -112,7 +90,7 @@ export function applyColorwayToData(
       typeof trace.line.color === 'string'
     ) {
       if (plotlyColors.has(trace.line.color.toUpperCase())) {
-        trace.line.color = colorMap.get(trace.line.color.toUpperCase());
+        delete trace.line.color;
       }
     }
   }