refactor scatter plot to separate code path (#43032)

* refactor scatter plot model to separate function * (2/3) refactor scatter plot option to separate function (#43033) * use getScatterPlotOption * use buildEChartsScatterSeries directly * remove hoveredSeriesDataKey from scatter * (3/3) improve chart model types (#43034) * create WaterfallChartModel type * create ScatterPlotModel type * rename BaseCartesianChartModel to CartesianChartModel * simplify chartModel creation in hook
metabase · May 23, 2024 · 1ea2b05 · 1ea2b05
1 parent ef28ff9
commit 1ea2b05
Show file tree

Hide file tree

Showing 22 changed files with 363 additions and 129 deletions.
diff --git a/frontend/src/metabase/static-viz/components/ScatterPlot/ScatterPlot.tsx b/frontend/src/metabase/static-viz/components/ScatterPlot/ScatterPlot.tsx
@@ -5,9 +5,9 @@ import type { StaticChartProps } from "metabase/static-viz/components/StaticVisu
 import { sanitizeSvgForBatik } from "metabase/static-viz/lib/svg";
 import { registerEChartsModules } from "metabase/visualizations/echarts";
 import { getChartMeasurements } from "metabase/visualizations/echarts/cartesian/chart-measurements";
-import { getCartesianChartModel } from "metabase/visualizations/echarts/cartesian/model";
 import { getLegendItems } from "metabase/visualizations/echarts/cartesian/model/legend";
-import { getCartesianChartOption } from "metabase/visualizations/echarts/cartesian/option";
+import { getScatterPlotModel } from "metabase/visualizations/echarts/cartesian/scatter/model";
+import { getScatterPlotOption } from "metabase/visualizations/echarts/cartesian/scatter/option";
 
 import { computeStaticComboChartSettings } from "../ComboChart/settings";
 import { Legend } from "../Legend";
@@ -35,7 +35,7 @@ export function ScatterPlot({
     renderingContext,
   );
 
-  const chartModel = getCartesianChartModel(
+  const chartModel = getScatterPlotModel(
     rawSeries,
     computedVisualizationSettings,
     renderingContext,
@@ -59,15 +59,14 @@ export function ScatterPlot({
     renderingContext,
   );
 
-  const option = getCartesianChartOption(
+  const option = getScatterPlotOption(
     chartModel,
     chartMeasurements,
     null,
     [],
     computedVisualizationSettings,
     width,
     false,
-    null,
     renderingContext,
   );
   chart.setOption(option);

diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/chart-measurements/index.ts b/frontend/src/metabase/visualizations/echarts/cartesian/chart-measurements/index.ts
@@ -3,7 +3,7 @@ import { X_AXIS_DATA_KEY } from "metabase/visualizations/echarts/cartesian/const
 import { CHART_STYLE } from "metabase/visualizations/echarts/cartesian/constants/style";
 import type {
   AxisFormatter,
-  BaseCartesianChartModel,
+  CartesianChartModel,
   ChartDataset,
   NumericAxisScaleTransforms,
   XAxisModel,
@@ -200,7 +200,7 @@ const X_LABEL_ROTATE_45_THRESHOLD_FACTOR = 2.1;
 const X_LABEL_ROTATE_90_THRESHOLD_FACTOR = 1.2;
 
 const getAutoAxisEnabledSetting = (
-  chartModel: BaseCartesianChartModel,
+  chartModel: CartesianChartModel,
   settings: ComputedVisualizationSettings,
   boundaryWidth: number,
   maxXTickWidth: number,
@@ -250,7 +250,7 @@ const getAutoAxisEnabledSetting = (
 };
 
 const getTicksDimensions = (
-  chartModel: BaseCartesianChartModel,
+  chartModel: CartesianChartModel,
   chartWidth: number,
   outerHeight: number,
   settings: ComputedVisualizationSettings,
@@ -348,7 +348,7 @@ const getTicksDimensions = (
 const TICK_OVERFLOW_BUFFER = 4;
 
 export const getChartPadding = (
-  chartModel: BaseCartesianChartModel,
+  chartModel: CartesianChartModel,
   settings: ComputedVisualizationSettings,
   ticksDimensions: TicksDimensions,
   axisEnabledSetting: ComputedVisualizationSettings["graph.x_axis.axis_enabled"],
@@ -478,7 +478,7 @@ export const getChartBounds = (
 };
 
 const getDimensionWidth = (
-  chartModel: BaseCartesianChartModel,
+  chartModel: CartesianChartModel,
   boundaryWidth: number,
 ) => {
   const { xAxisModel } = chartModel;
@@ -525,7 +525,7 @@ const areHorizontalXAxisTicksOverlapping = (
 };
 
 export const getChartMeasurements = (
-  chartModel: BaseCartesianChartModel,
+  chartModel: CartesianChartModel,
   settings: ComputedVisualizationSettings,
   hasTimelineEvents: boolean,
   width: number,

diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/model/dataset.ts b/frontend/src/metabase/visualizations/echarts/cartesian/model/dataset.ts
@@ -1,6 +1,6 @@
 import { t } from "ttag";
 
-import { getObjectKeys, getObjectValues } from "metabase/lib/objects";
+import { getObjectKeys } from "metabase/lib/objects";
 import { parseTimestamp } from "metabase/lib/time-dayjs";
 import { checkNumber, isNotNull } from "metabase/lib/types";
 import { isEmpty } from "metabase/lib/validate";
@@ -875,28 +875,3 @@ export const getCardsColumnByDataKeyMap = (
     return { ...acc, ...cardColumnByDataKeyMap };
   }, {} as Record<DataKey, DatasetColumn>);
 };
-
-export const getBubbleSizeDomain = (
-  seriesModels: SeriesModel[],
-  dataset: ChartDataset,
-): Extent | null => {
-  const bubbleSizeDataKeys = seriesModels
-    .map(seriesModel =>
-      "bubbleSizeDataKey" in seriesModel &&
-      seriesModel.bubbleSizeDataKey != null
-        ? seriesModel.bubbleSizeDataKey
-        : null,
-    )
-    .filter(isNotNull);
-
-  if (bubbleSizeDataKeys.length === 0) {
-    return null;
-  }
-
-  const bubbleSizeMaxValues = getObjectValues(
-    getDatasetExtents(bubbleSizeDataKeys, dataset),
-  ).map(extent => extent[1]);
-  const bubbleSizeDomainMax = Math.max(...bubbleSizeMaxValues);
-
-  return [0, bubbleSizeDomainMax];
-};
diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/model/index.ts b/frontend/src/metabase/visualizations/echarts/cartesian/model/index.ts
@@ -3,7 +3,6 @@ import {
   getYAxesModels,
 } from "metabase/visualizations/echarts/cartesian/model/axis";
 import {
-  getBubbleSizeDomain,
   getCardsColumnByDataKeyMap,
   getJoinedCardsDataset,
   getSortedSeriesModels,
@@ -20,7 +19,6 @@ import type {
   CartesianChartModel,
   ShowWarning,
 } from "metabase/visualizations/echarts/cartesian/model/types";
-import { getScatterPlotDataset } from "metabase/visualizations/echarts/cartesian/scatter/model";
 import { getCartesianChartColumns } from "metabase/visualizations/lib/graph/columns";
 import { getSingleSeriesDimensionsAndMetrics } from "metabase/visualizations/lib/utils";
 import type {
@@ -33,8 +31,6 @@ import { getStackModels } from "./stack";
 import { getAxisTransforms } from "./transforms";
 import { getTrendLines } from "./trend-line";
 
-const SUPPORTED_AUTO_SPLIT_TYPES = ["line", "area", "bar", "combo"];
-
 // HACK: when multiple cards (datasets) are combined on a single dashboard card
 // the settings prop of the visualization contains only one set of metrics and dimensions
 // which by design is not sufficient for multiple cards. At the same time, not all cards settings
@@ -103,15 +99,16 @@ export const getCartesianChartModel = (
     ? unsortedSeriesModels
     : getSortedSeriesModels(unsortedSeriesModels, settings);
 
-  let dataset;
-  switch (rawSeries[0].card.display) {
-    case "scatter":
-      dataset = getScatterPlotDataset(rawSeries, cardsColumns);
-      break;
-    default:
-      dataset = getJoinedCardsDataset(rawSeries, cardsColumns, showWarning);
-  }
-  dataset = sortDataset(dataset, settings["graph.x_axis.scale"], showWarning);
+  const unsortedDataset = getJoinedCardsDataset(
+    rawSeries,
+    cardsColumns,
+    showWarning,
+  );
+  const dataset = sortDataset(
+    unsortedDataset,
+    settings["graph.x_axis.scale"],
+    showWarning,
+  );
 
   const xAxisModel = getXAxisModel(
     dimensionModel,
@@ -137,10 +134,6 @@ export const getCartesianChartModel = (
     showWarning,
   );
 
-  const isAutoSplitSupported = SUPPORTED_AUTO_SPLIT_TYPES.includes(
-    rawSeries[0].card.display,
-  );
-
   const { formatters: seriesLabelsFormatters, compactSeriesDataKeys } =
     getSeriesLabelsFormatters(
       seriesModels,
@@ -163,7 +156,7 @@ export const getCartesianChartModel = (
     transformedDataset,
     settings,
     columnByDataKey,
-    isAutoSplitSupported,
+    true,
     stackModels,
     [...compactSeriesDataKeys, ...compactStackedSeriesDataKeys],
     renderingContext,
@@ -192,7 +185,6 @@ export const getCartesianChartModel = (
     leftAxisModel,
     rightAxisModel,
     trendLinesModel,
-    bubbleSizeDomain: getBubbleSizeDomain(seriesModels, transformedDataset),
     seriesLabelsFormatters,
     stackedLabelsFormatters,
   };

diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/model/types.ts b/frontend/src/metabase/visualizations/echarts/cartesian/model/types.ts
@@ -192,7 +192,7 @@ export type StackModel = {
   seriesKeys: DataKey[];
 };
 
-export type BaseCartesianChartModel = {
+export type CartesianChartModel = {
   dimensionModel: DimensionModel;
   seriesModels: SeriesModel[];
   dataset: ChartDataset;
@@ -213,13 +213,16 @@ export type BaseCartesianChartModel = {
   trendLinesModel?: TrendLinesModel;
   seriesLabelsFormatters?: SeriesFormatters;
   stackedLabelsFormatters?: StackedSeriesFormatters;
-  waterfallLabelFormatter?: LabelFormatter;
 };
 
-export type CartesianChartModel = BaseCartesianChartModel & {
+export type ScatterPlotModel = CartesianChartModel & {
   bubbleSizeDomain: Extent | null;
 };
 
+export type WaterfallChartModel = CartesianChartModel & {
+  waterfallLabelFormatter: LabelFormatter | undefined;
+};
+
 export type ShowWarning = (warning: string) => void;
 
 export type LegendItem = {

diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/option/axis.ts b/frontend/src/metabase/visualizations/echarts/cartesian/option/axis.ts
@@ -4,7 +4,7 @@ import type { AxisBaseOptionCommon } from "echarts/types/src/coord/axisCommonTyp
 import { parseNumberValue } from "metabase/lib/number";
 import { CHART_STYLE } from "metabase/visualizations/echarts/cartesian/constants/style";
 import type {
-  BaseCartesianChartModel,
+  CartesianChartModel,
   DataKey,
   Extent,
   NumericAxisScaleTransforms,
@@ -107,7 +107,7 @@ export const getDimensionTicksDefaultOption = (
 };
 
 const getHistogramTicksOptions = (
-  chartModel: BaseCartesianChartModel,
+  chartModel: CartesianChartModel,
   settings: ComputedVisualizationSettings,
   chartMeasurements: ChartMeasurements,
 ) => {
@@ -185,7 +185,7 @@ const getCommonDimensionAxisOptions = (
 };
 
 export const buildDimensionAxis = (
-  chartModel: BaseCartesianChartModel,
+  chartModel: CartesianChartModel,
   width: number,
   settings: ComputedVisualizationSettings,
   chartMeasurements: ChartMeasurements,
@@ -308,7 +308,7 @@ export const buildTimeSeriesDimensionAxis = (
 };
 
 export const buildCategoricalDimensionAxis = (
-  chartModel: BaseCartesianChartModel,
+  chartModel: CartesianChartModel,
   originalSettings: ComputedVisualizationSettings,
   chartMeasurements: ChartMeasurements,
   renderingContext: RenderingContext,
@@ -411,7 +411,7 @@ export const buildMetricAxis = (
 };
 
 const buildMetricsAxes = (
-  chartModel: BaseCartesianChartModel,
+  chartModel: CartesianChartModel,
   chartMeasurements: ChartMeasurements,
   settings: ComputedVisualizationSettings,
   hoveredSeriesDataKey: DataKey | null,
@@ -455,7 +455,7 @@ const buildMetricsAxes = (
 };
 
 export const buildAxes = (
-  chartModel: BaseCartesianChartModel,
+  chartModel: CartesianChartModel,
   width: number,
   chartMeasurements: ChartMeasurements,
   settings: ComputedVisualizationSettings,

diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/option/goal-line.ts b/frontend/src/metabase/visualizations/echarts/cartesian/option/goal-line.ts
@@ -8,7 +8,7 @@ import type {
 import type { EChartsCartesianCoordinateSystem } from "../../types";
 import { GOAL_LINE_SERIES_ID, X_AXIS_DATA_KEY } from "../constants/dataset";
 import { CHART_STYLE } from "../constants/style";
-import type { CartesianChartModel, ChartDataset } from "../model/types";
+import type { ChartDataset, CartesianChartModel } from "../model/types";
 
 export const GOAL_LINE_DASH = [3, 4];
 

diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/option/index.ts b/frontend/src/metabase/visualizations/echarts/cartesian/option/index.ts
@@ -7,8 +7,8 @@ import {
   X_AXIS_DATA_KEY,
 } from "metabase/visualizations/echarts/cartesian/constants/dataset";
 import type {
-  CartesianChartModel,
   DataKey,
+  CartesianChartModel,
 } from "metabase/visualizations/echarts/cartesian/model/types";
 import { buildAxes } from "metabase/visualizations/echarts/cartesian/option/axis";
 import { buildEChartsSeries } from "metabase/visualizations/echarts/cartesian/option/series";
@@ -61,7 +61,6 @@ export const getCartesianChartOption = (
       )
     : null;
 
-  // series option
   const dataSeriesOptions = buildEChartsSeries(
     chartModel,
     settings,

diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/option/series.ts b/frontend/src/metabase/visualizations/echarts/cartesian/option/series.ts
@@ -16,7 +16,6 @@ import {
 } from "metabase/visualizations/echarts/cartesian/constants/style";
 import type {
   SeriesModel,
-  CartesianChartModel,
   DataKey,
   StackTotalDataKey,
   ChartDataset,
@@ -27,6 +26,7 @@ import type {
   NumericAxisScaleTransforms,
   LabelFormatter,
   StackModel,
+  CartesianChartModel,
 } from "metabase/visualizations/echarts/cartesian/model/types";
 import type { EChartsSeriesOption } from "metabase/visualizations/echarts/cartesian/option/types";
 import type {
@@ -42,7 +42,6 @@ import {
   isTimeSeriesAxis,
 } from "../model/guards";
 import { getStackTotalValue } from "../model/series";
-import { buildEChartsScatterSeries } from "../scatter/series";
 
 import { getSeriesYAxisIndex } from "./utils";
 
@@ -614,13 +613,6 @@ export const buildEChartsSeries = (
             chartModel?.seriesLabelsFormatters?.[seriesModel.dataKey],
             renderingContext,
           );
-        case "scatter":
-          return buildEChartsScatterSeries(
-            seriesModel,
-            chartModel.bubbleSizeDomain,
-            yAxisIndex,
-            renderingContext,
-          );
       }
     })
     .flat()
@@ -635,11 +627,7 @@ export const buildEChartsSeries = (
         chartModel,
         chartModel.yAxisScaleTransforms,
         settings,
-        // It's guranteed that no series here will be scatter, since with
-        // scatter plots the `stackable.stack_type` is undefined. We can maybe
-        // remove this later after refactoring the scatter implementation to a
-        // separate codepath.
-        series as (LineSeriesOption | BarSeriesOption)[],
+        series,
       ),
     );
   }

diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/option/utils.ts b/frontend/src/metabase/visualizations/echarts/cartesian/option/utils.ts
@@ -1,4 +1,4 @@
-import type { CartesianChartModel, DataKey } from "../model/types";
+import type { DataKey, CartesianChartModel } from "../model/types";
 
 export function getSeriesYAxisIndex(
   dataKey: DataKey,

diff --git a/...ations/echarts/cartesian/scatter/model.ts → ...charts/cartesian/scatter/model/dataset.ts b/...ations/echarts/cartesian/scatter/model.ts → ...charts/cartesian/scatter/model/dataset.ts
@@ -2,8 +2,8 @@ import { X_AXIS_DATA_KEY } from "metabase/visualizations/echarts/cartesian/const
 import type { CartesianChartColumns } from "metabase/visualizations/lib/graph/columns";
 import type { RawSeries } from "metabase-types/api";
 
-import { getDatasetKey } from "../model/dataset";
-import type { ChartDataset, Datum } from "../model/types";
+import { getDatasetKey } from "../../model/dataset";
+import type { ChartDataset, Datum } from "../../model/types";
 
 export function getScatterPlotDataset(
   rawSeries: RawSeries,