Fix data labels too dense with large single series (#42985)

* remove unused variable * conditionally render labels sparsely if large amount of data points * update loki snapshots * refactor data label formatter conditions * setup intelligent label plotting * handle the case when there are no labels * add sparse data label rendering to bar charts * do not compute if no labels need to be shown or hidden * add sparse data labeling to waterfall charts * adjustment for large series waterfall labels * revert change * refactor name * add sparse labels to stacked bar and area * improve scaleFactor * fix type errors * remove unused labelFormatter * update snapshots * patch ECharts - labels with no text, i.e. "", were still being considered as plot-able labels which was causing labels with real text "asdfec.." to be hidden because of overlap. - This patch makes it so that labels with no text, i.e. "", are not considered to be candidates for overlap checking. * update loki snapshots * only loop over dataset once for getWaterfallChartDataDensity * improve performance of chart data density calculations * update loki snapshots * update E2E spec * update E2E spec * increase cartesian label density allotment * update loki snapshots * update loki snapshots * comment out node_modules cache since it cannot be skipped by commit and is breaking CI * fix type errors * Reset node modules if patches are changed * refactor type naming --------- Co-authored-by: Uladzimir Havenchyk <uladzimir.dev@gmail.com> Co-authored-by: Uladzimir Havenchyk <125459446+uladzimirdev@users.noreply.github.com>
metabase · Jun 1, 2024 · 72443c6 · 72443c6
1 parent 2c1333e
commit 72443c6
Show file tree

Hide file tree

Showing 45 changed files with 607 additions and 167 deletions.
diff --git a/.github/actions/prepare-frontend/action.yml b/.github/actions/prepare-frontend/action.yml
@@ -23,6 +23,6 @@ runs:
       if: ${{ !contains(github.event.head_commit.message, '[ci nocache]') }}
       with:
         path: node_modules
-        key: ${{ runner.os }}-node-modules-${{ hashFiles('**/yarn.lock') }}
+        key: ${{ runner.os }}-node-modules-${{ hashFiles('**/yarn.lock', '**/patches/*.patch') }}
     - run: yarn install --frozen-lockfile --prefer-offline
       shell: bash
diff --git a/...reference/chrome_laptop_static_viz_ComboChart_Area_Stacked_Auto_Data_Labels.png b/...reference/chrome_laptop_static_viz_ComboChart_Area_Stacked_Auto_Data_Labels.png
diff --git a/...tatic_viz_ComboChart_Bars_Breakout_Sorted_With_Negative_Values_Power_Y_Axis.png b/...tatic_viz_ComboChart_Bars_Breakout_Sorted_With_Negative_Values_Power_Y_Axis.png
diff --git a/...ence/chrome_laptop_static_viz_ComboChart_Incorrect_Label_Y_Axis_Split_41285.png b/...ence/chrome_laptop_static_viz_ComboChart_Incorrect_Label_Y_Axis_Split_41285.png
diff --git a/...nce/chrome_laptop_static_viz_ScatterPlot_Auto_Y_Axis_Exclude_Zero_With_Goal.png b/...nce/chrome_laptop_static_viz_ScatterPlot_Auto_Y_Axis_Exclude_Zero_With_Goal.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Bubble_Size.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Bubble_Size.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Custom_Y_Axis_Range.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Custom_Y_Axis_Range.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Default.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Default.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Goal_Line.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Goal_Line.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Histogram_X_Scale.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Histogram_X_Scale.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Log_X_Scale.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Log_X_Scale.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Log_X_Scale_At_One.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Log_X_Scale_At_One.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Multi_Dimension_Breakout.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Multi_Dimension_Breakout.png
diff --git a/...e/chrome_laptop_static_viz_ScatterPlot_Multi_Dimension_Breakout_Bubble_Size.png b/...e/chrome_laptop_static_viz_ScatterPlot_Multi_Dimension_Breakout_Bubble_Size.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Multi_Metric_Series.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Multi_Metric_Series.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Ordinal_X_Scale.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Ordinal_X_Scale.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Power_X_Scale.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Power_X_Scale.png
diff --git a/...i/reference/chrome_laptop_static_viz_ScatterPlot_Power_X_Scale_Multi_Series.png b/...i/reference/chrome_laptop_static_viz_ScatterPlot_Power_X_Scale_Multi_Series.png
diff --git a/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Timeseries_X_Scale.png b/.loki/reference/chrome_laptop_static_viz_ScatterPlot_Timeseries_X_Scale.png
diff --git a/...rence/chrome_laptop_static_viz_WaterfallChart_Data_Labels_Column_Formatting.png b/...rence/chrome_laptop_static_viz_WaterfallChart_Data_Labels_Column_Formatting.png
diff --git a/.loki/reference/chrome_laptop_static_viz_WaterfallChart_Data_Labels_Timeseries.png b/.loki/reference/chrome_laptop_static_viz_WaterfallChart_Data_Labels_Timeseries.png
diff --git a/.loki/reference/chrome_laptop_static_viz_WaterfallChart_Log_Y_Scale.png b/.loki/reference/chrome_laptop_static_viz_WaterfallChart_Log_Y_Scale.png
diff --git a/.loki/reference/chrome_laptop_static_viz_WaterfallChart_Log_Y_Scale_Negative.png b/.loki/reference/chrome_laptop_static_viz_WaterfallChart_Log_Y_Scale_Negative.png
diff --git a/...reference/chrome_laptop_static_viz_WaterfallChart_Pow_Y_Scale_Negative_Only.png b/...reference/chrome_laptop_static_viz_WaterfallChart_Pow_Y_Scale_Negative_Only.png
diff --git a/e2e/test/scenarios/visualizations-charts/combo.cy.spec.js b/e2e/test/scenarios/visualizations-charts/combo.cy.spec.js
@@ -44,7 +44,7 @@ describe("scenarios > visualizations > combo", () => {
       },
     });
     // First value label on the chart
-    cy.findAllByText("440.52");
+    cy.findAllByText("390.99");
   });
 
   it("should support stacking", () => {

diff --git a/e2e/test/scenarios/visualizations-charts/waterfall.cy.spec.js b/e2e/test/scenarios/visualizations-charts/waterfall.cy.spec.js
@@ -259,11 +259,23 @@ describe("scenarios > visualizations > waterfall", () => {
       },
     });
 
+    // the null data label which should exist at index -3
+    // should now display no label. so the label at index -3 should be the label
+    // before the null data point
     getWaterfallDataLabels()
       .as("labels")
       .eq(-3)
       .invoke("text")
-      .should("eq", " ");
+      .should("eq", "0.1");
+
+    // but the x-axis label and area should still be shown for the null data point
+    echartsContainer().findByText("f");
+
+    getWaterfallDataLabels()
+      .as("labels")
+      .eq(-2)
+      .invoke("text")
+      .should("eq", "(2)");
 
     cy.get("@labels").last().invoke("text").should("eq", "0.1");
   });

diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/chart-measurements/index.ts b/frontend/src/metabase/visualizations/echarts/cartesian/chart-measurements/index.ts
@@ -6,7 +6,7 @@ import {
 import { CHART_STYLE } from "metabase/visualizations/echarts/cartesian/constants/style";
 import type {
   AxisFormatter,
-  CartesianChartModel,
+  BaseCartesianChartModel,
   ChartDataset,
   NumericAxisScaleTransforms,
   StackModel,
@@ -204,7 +204,7 @@ const X_LABEL_ROTATE_45_THRESHOLD_FACTOR = 2.1;
 const X_LABEL_ROTATE_90_THRESHOLD_FACTOR = 1.2;
 
 const getAutoAxisEnabledSetting = (
-  chartModel: CartesianChartModel,
+  chartModel: BaseCartesianChartModel,
   settings: ComputedVisualizationSettings,
   boundaryWidth: number,
   maxXTickWidth: number,
@@ -254,7 +254,7 @@ const getAutoAxisEnabledSetting = (
 };
 
 const getTicksDimensions = (
-  chartModel: CartesianChartModel,
+  chartModel: BaseCartesianChartModel,
   chartWidth: number,
   outerHeight: number,
   settings: ComputedVisualizationSettings,
@@ -352,7 +352,7 @@ const getTicksDimensions = (
 const TICK_OVERFLOW_BUFFER = 4;
 
 export const getChartPadding = (
-  chartModel: CartesianChartModel,
+  chartModel: BaseCartesianChartModel,
   settings: ComputedVisualizationSettings,
   ticksDimensions: TicksDimensions,
   axisEnabledSetting: ComputedVisualizationSettings["graph.x_axis.axis_enabled"],
@@ -482,7 +482,7 @@ export const getChartBounds = (
 };
 
 const getDimensionWidth = (
-  chartModel: CartesianChartModel,
+  chartModel: BaseCartesianChartModel,
   boundaryWidth: number,
 ) => {
   const { xAxisModel } = chartModel;
@@ -529,7 +529,7 @@ const areHorizontalXAxisTicksOverlapping = (
 };
 
 const countFittingLabels = (
-  chartModel: CartesianChartModel,
+  chartModel: BaseCartesianChartModel,
   barStack: StackModel,
   barWidth: number,
   renderingContext: RenderingContext,
@@ -586,7 +586,7 @@ const BAR_WIDTH_PRECISION = 0.85;
 const HORIZONTAL_LABELS_COUNT_THRESHOLD = 0.8;
 
 const getStackedBarTicksRotation = (
-  chartModel: CartesianChartModel,
+  chartModel: BaseCartesianChartModel,
   boundaryWidth: number,
   renderingContext: RenderingContext,
 ) => {
@@ -627,7 +627,7 @@ const getStackedBarTicksRotation = (
 };
 
 export const getChartMeasurements = (
-  chartModel: CartesianChartModel,
+  chartModel: BaseCartesianChartModel,
   settings: ComputedVisualizationSettings,
   hasTimelineEvents: boolean,
   width: number,

diff --git a/frontend/src/metabase/visualizations/echarts/cartesian/model/index.ts b/frontend/src/metabase/visualizations/echarts/cartesian/model/index.ts
@@ -11,6 +11,7 @@ import {
 } from "metabase/visualizations/echarts/cartesian/model/dataset";
 import {
   getCardsSeriesModels,
+  getComboChartDataDensity,
   getDimensionModel,
   getSeriesLabelsFormatters,
   getStackedLabelsFormatters,
@@ -152,6 +153,16 @@ export const getCartesianChartModel = (
       renderingContext,
     );
 
+  const dataDensity = getComboChartDataDensity(
+    seriesModels,
+    stackModels,
+    transformedDataset,
+    seriesLabelsFormatters,
+    stackedLabelsFormatters,
+    settings,
+    renderingContext,
+  );
+
   const { leftAxisModel, rightAxisModel } = getYAxesModels(
     seriesModels,
     transformedDataset,
@@ -188,5 +199,6 @@ export const getCartesianChartModel = (
     trendLinesModel,
     seriesLabelsFormatters,
     stackedLabelsFormatters,
+    dataDensity,
   };
 };