fix aggressive y-axis splitting in line, area, bar charts (#29254)

* fix agressive y-axis splitting in charts

* e2e tests

* check if col settings are different

* use `settings.column` instead of redefining default settings

* refactor for feedback

e2e/test/scenarios/visualizations/bar_chart.cy.spec.js

@@ -225,6 +225,59 @@ describe("scenarios > visualizations > bar chart", () => {
     });
   });
 
+  // Note (EmmadUsmani): see `line_chart.cy.spec.js` for more test cases of this
+  describe("with split y-axis (metabase#12939)", () => {
+    it("should split the y-axis when column settings differ", () => {
+      visitQuestionAdhoc({
+        dataset_query: {
+          type: "query",
+          query: {
+            "source-table": ORDERS_ID,
+            aggregation: [
+              ["avg", ["field", ORDERS.TOTAL, null]],
+              ["min", ["field", ORDERS.TOTAL, null]],
+            ],
+            breakout: [
+              ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
+            ],
+          },
+          database: SAMPLE_DB_ID,
+        },
+        display: "bar",
+        visualization_settings: {
+          column_settings: {
+            '["name","avg"]': { number_style: "decimal" },
+            '["name","min"]': { number_style: "percent" },
+          },
+        },
+      });
+
+      cy.get("g.axis.yr").should("be.visible");
+    });
+
+    it("should not split the y-axis when semantic_type, column settings are same and values are not far", () => {
+      visitQuestionAdhoc({
+        dataset_query: {
+          type: "query",
+          query: {
+            "source-table": ORDERS_ID,
+            aggregation: [
+              ["avg", ["field", ORDERS.TOTAL, null]],
+              ["min", ["field", ORDERS.TOTAL, null]],
+            ],
+            breakout: [
+              ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
+            ],
+          },
+          database: SAMPLE_DB_ID,
+        },
+        display: "bar",
+      });
+
+      cy.get("g.axis.yr").should("not.exist");
+    });
+  });
+
   it("supports up to 100 series (metabase#28796)", () => {
     visitQuestionAdhoc({
       display: "bar",

e2e/test/scenarios/visualizations/line_chart.cy.spec.js

@@ -10,7 +10,8 @@ import {
 import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
 import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
 
-const { ORDERS, ORDERS_ID, PRODUCTS, PRODUCTS_ID } = SAMPLE_DATABASE;
+const { ORDERS, ORDERS_ID, PRODUCTS, PRODUCTS_ID, PEOPLE, PEOPLE_ID } =
+  SAMPLE_DATABASE;
 
 const Y_AXIS_RIGHT_SELECTOR = ".axis.yr";
 
@@ -201,6 +202,99 @@ describe("scenarios > visualizations > line chart", () => {
     cy.get(`.sub._0`).find("circle").should("have.length", 2);
   });
 
+  describe("y-axis splitting (metabase#12939)", () => {
+    it("should not split the y-axis when columns are of the same semantic_type and have close values", () => {
+      visitQuestionAdhoc({
+        dataset_query: {
+          type: "query",
+          query: {
+            "source-table": ORDERS_ID,
+            aggregation: [
+              ["avg", ["field", ORDERS.TOTAL, null]],
+              ["min", ["field", ORDERS.TOTAL, null]],
+            ],
+            breakout: [
+              ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
+            ],
+          },
+          database: SAMPLE_DB_ID,
+        },
+        display: "line",
+      });
+
+      cy.get("g.axis.yr").should("not.exist");
+    });
+
+    it("should split the y-axis when columns are of different semantic_type", () => {
+      visitQuestionAdhoc({
+        dataset_query: {
+          type: "query",
+          query: {
+            "source-table": PEOPLE_ID,
+            aggregation: [
+              ["avg", ["field", PEOPLE.LATITUDE, null]],
+              ["avg", ["field", PEOPLE.LONGITUDE, null]],
+            ],
+            breakout: [
+              ["field", PEOPLE.CREATED_AT, { "temporal-unit": "month" }],
+            ],
+          },
+          database: SAMPLE_DB_ID,
+        },
+        display: "line",
+      });
+
+      cy.get("g.axis.yr").should("be.visible");
+    });
+
+    it("should split the y-six when columns are of the same semantic_type but have far values", () => {
+      visitQuestionAdhoc({
+        dataset_query: {
+          type: "query",
+          query: {
+            "source-table": ORDERS_ID,
+            aggregation: [
+              ["sum", ["field", ORDERS.TOTAL, null]],
+              ["min", ["field", ORDERS.TOTAL, null]],
+            ],
+            breakout: [
+              ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
+            ],
+          },
+          database: SAMPLE_DB_ID,
+        },
+        display: "line",
+      });
+
+      cy.get("g.axis.yr").should("be.visible");
+    });
+
+    it("should not split the y-axis when the setting is disabled", () => {
+      visitQuestionAdhoc({
+        dataset_query: {
+          type: "query",
+          query: {
+            "source-table": ORDERS_ID,
+            aggregation: [
+              ["sum", ["field", ORDERS.TOTAL, null]],
+              ["min", ["field", ORDERS.TOTAL, null]],
+            ],
+            breakout: [
+              ["field", ORDERS.CREATED_AT, { "temporal-unit": "month" }],
+            ],
+          },
+          database: SAMPLE_DB_ID,
+        },
+        display: "line",
+        visualization_settings: {
+          "graph.y_axis.auto_split": false,
+        },
+      });
+
+      cy.get("g.axis.yr").should("not.exist");
+    });
+  });
+
   describe("tooltip of combined dashboard cards (multi-series) should show the correct column title (metabase#16249", () => {
     const RENAMED_FIRST_SERIES = "Foo";
     const RENAMED_SECOND_SERIES = "Bar";

frontend/src/metabase/visualizations/lib/LineAreaBarRenderer.js

@@ -59,6 +59,7 @@ import {
   isMultiCardSeries,
   hasClickBehavior,
   replaceNullValuesForOrdinal,
+  shouldSplitYAxis,
 } from "./renderer_utils";
 
 import lineAndBarOnRender from "./LineAreaBarPostRender";
@@ -290,15 +291,12 @@ function getYAxisSplit(
     }
   }
 
-  // don't auto-split if the metric columns are all identical, i.e. it's a breakout multiseries
-  const hasDifferentYAxisColumns =
-    _.uniq(series.map(s => JSON.stringify(s.data.cols[1]))).length > 1;
   if (
-    !isScalarSeries &&
-    chartType !== "scatter" &&
-    !isStacked(settings, datas) &&
-    hasDifferentYAxisColumns &&
-    settings["graph.y_axis.auto_split"] !== false
+    shouldSplitYAxis(
+      { settings, chartType, isScalarSeries, series },
+      datas,
+      yExtents,
+    )
   ) {
     // NOTE: this version computes the split after assigning fixed left/right
     // which causes other series to move around when changing the setting

frontend/src/metabase/visualizations/lib/renderer_utils.js

@@ -22,6 +22,7 @@ import { computeNumericDataInverval, dimensionIsNumeric } from "./numeric";
 
 import { getAvailableCanvasWidth, getAvailableCanvasHeight } from "./utils";
 import { invalidDateWarning, nullDimensionWarning } from "./warnings";
+import { getLineAreaBarComparisonSettings } from "./settings";
 
 export function initChart(chart, element) {
   // set the bounds
@@ -352,6 +353,46 @@ export function xValueForWaterfallTotal({ settings, series }) {
   return TOTAL_ORDINAL_VALUE;
 }
 
+export function shouldSplitYAxis(
+  { settings, chartType, isScalarSeries, series },
+  datas,
+  yExtents,
+) {
+  if (
+    isScalarSeries ||
+    chartType === "scatter" ||
+    settings["graph.y_axis.auto_split"] === false ||
+    isStacked(settings, datas)
+  ) {
+    return false;
+  }
+
+  const hasDifferentYAxisColTypes =
+    _.uniq(series.map(s => s.data.cols[1].semantic_type)).length > 1;
+  if (hasDifferentYAxisColTypes) {
+    return true;
+  }
+
+  const columnSettings = series.map(s =>
+    getLineAreaBarComparisonSettings(settings.column(s.data.cols[1])),
+  );
+  const hasDifferentColumnSettings = columnSettings.some(s1 =>
+    columnSettings.some(s2 => !_.isEqual(s1, s2)),
+  );
+  if (hasDifferentColumnSettings) {
+    return true;
+  }
+
+  const minRange = Math.min(...yExtents.map(extent => extent[1] - extent[0]));
+  const maxExtent = Math.max(...yExtents.map(extent => extent[1]));
+  const minExtent = Math.min(...yExtents.map(extent => extent[0]));
+  const chartRange = maxExtent - minExtent;
+
+  // Note (EmmadUsmani): When the series with the smallest range is less than 5%
+  // of the chart's total range, we split the y-axis so it doesn't look too small.
+  return minRange / chartRange <= 0.05;
+}
+
 /************************************************************ PROPERTIES ************************************************************/
 
 export const isTimeseries = settings =>

frontend/src/metabase/visualizations/lib/settings.js

 /* eslint-disable react/prop-types */
 import { getIn } from "icepick";
 
+import _ from "underscore";
 import ChartSettingInput from "metabase/visualizations/components/settings/ChartSettingInput";
 import ChartSettingInputGroup from "metabase/visualizations/components/settings/ChartSettingInputGroup";
 import ChartSettingInputNumeric from "metabase/visualizations/components/settings/ChartSettingInputNumeric";
@@ -276,3 +277,26 @@ function getColumnClickBehavior(columnSettings) {
       };
     }, null);
 }
+
+const KEYS_TO_COMPARE = new Set([
+  "number_style",
+  "currency",
+  "currency_style",
+  "number_separators",
+  "decimals",
+  "scale",
+  "prefix",
+  "suffix",
+]);
+
+export function getLineAreaBarComparisonSettings(columnSettings) {
+  return _.pick(columnSettings, (value, key) => {
+    if (!KEYS_TO_COMPARE.has(key)) {
+      return false;
+    }
+    if ((key === "prefix" || key === "suffix") && value === "") {
+      return false;
+    }
+    return true;
+  });
+}