From ea901a03395777d3472c718c3c7159de571655bf Mon Sep 17 00:00:00 2001
From: Emmad Usmani <emmadusmani@berkeley.edu>
Date: Sun, 19 Mar 2023 17:31:26 -0700
Subject: [PATCH] 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
---
.../visualizations/bar_chart.cy.spec.js | 53 ++++++++++
.../visualizations/line_chart.cy.spec.js | 96 ++++++++++++++++++-
.../visualizations/lib/LineAreaBarRenderer.js | 14 ++-
.../visualizations/lib/renderer_utils.js | 41 ++++++++
.../metabase/visualizations/lib/settings.js | 24 +++++
5 files changed, 219 insertions(+), 9 deletions(-)
diff --git a/e2e/test/scenarios/visualizations/bar_chart.cy.spec.js b/e2e/test/scenarios/visualizations/bar_chart.cy.spec.js
index 67bf128b82b..226dacfe82e 100644
--- a/e2e/test/scenarios/visualizations/bar_chart.cy.spec.js
+++ b/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",
diff --git a/e2e/test/scenarios/visualizations/line_chart.cy.spec.js b/e2e/test/scenarios/visualizations/line_chart.cy.spec.js
index 8d0eaf8868d..7c30edadf9a 100644
--- a/e2e/test/scenarios/visualizations/line_chart.cy.spec.js
+++ b/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";
diff --git a/frontend/src/metabase/visualizations/lib/LineAreaBarRenderer.js b/frontend/src/metabase/visualizations/lib/LineAreaBarRenderer.js
index c3a9f327ccb..4ccb0838144 100644
--- a/frontend/src/metabase/visualizations/lib/LineAreaBarRenderer.js
+++ b/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
diff --git a/frontend/src/metabase/visualizations/lib/renderer_utils.js b/frontend/src/metabase/visualizations/lib/renderer_utils.js
index a562c862d74..09993917da0 100644
--- a/frontend/src/metabase/visualizations/lib/renderer_utils.js
+++ b/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 =>
diff --git a/frontend/src/metabase/visualizations/lib/settings.js b/frontend/src/metabase/visualizations/lib/settings.js
index b8fbbc0e6f0..e52e625fe76 100644
--- a/frontend/src/metabase/visualizations/lib/settings.js
+++ b/frontend/src/metabase/visualizations/lib/settings.js
@@ -1,6 +1,7 @@
/* 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;
+ });
+}
--
GitLab