From 69d1b3c69f8101ce19314c3c11b7ea7a10e37253 Mon Sep 17 00:00:00 2001
From: Ryan Laurie <30528226+iethree@users.noreply.github.com>
Date: Mon, 16 Jan 2023 15:41:48 -0700
Subject: [PATCH] Dynamic pivot table cell size (#27613)
* add PivotTable unit tests
* obey the linter
* measure leftHeader cell content
* test cell data width detection
* obey the linter
* fix bad rebase
---
.../visualizations/PivotTable/PivotTable.jsx | 1 +
.../visualizations/PivotTable/constants.ts | 2 +
.../visualizations/PivotTable/types.ts | 20 +++
.../visualizations/PivotTable/utils.ts | 81 ++++++++++--
.../PivotTable/utils.unit.spec.ts | 120 +++++++++++++++++-
5 files changed, 211 insertions(+), 13 deletions(-)
diff --git a/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.jsx b/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.jsx
index c2786eee63f..7f4df36a6cb 100644
--- a/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.jsx
+++ b/frontend/src/metabase/visualizations/visualizations/PivotTable/PivotTable.jsx
@@ -360,6 +360,7 @@ class PivotTable extends Component {
const { leftHeaderWidths, totalHeaderWidths } = getLeftHeaderWidths({
rowIndexes: rowIndexes ?? [],
getColumnTitle: idx => this.getColumnTitle(idx),
+ leftHeaderItems,
fontFamily: fontFamily,
});
diff --git a/frontend/src/metabase/visualizations/visualizations/PivotTable/constants.ts b/frontend/src/metabase/visualizations/visualizations/PivotTable/constants.ts
index f5e507fc422..3ea138845d0 100644
--- a/frontend/src/metabase/visualizations/visualizations/PivotTable/constants.ts
+++ b/frontend/src/metabase/visualizations/visualizations/PivotTable/constants.ts
@@ -16,3 +16,5 @@ export const MAX_HEADER_CELL_WIDTH =
// the left header has some additional padding on the left to align with the title
export const LEFT_HEADER_LEFT_SPACING = 24;
+
+export const MAX_ROWS_TO_MEASURE = 100;
diff --git a/frontend/src/metabase/visualizations/visualizations/PivotTable/types.ts b/frontend/src/metabase/visualizations/visualizations/PivotTable/types.ts
index 215adc9e0ee..3da159de665 100644
--- a/frontend/src/metabase/visualizations/visualizations/PivotTable/types.ts
+++ b/frontend/src/metabase/visualizations/visualizations/PivotTable/types.ts
@@ -1,4 +1,5 @@
import type { FieldReference, AggregationReference } from "metabase-types/api";
+import type { Column } from "metabase-types/types/Dataset";
export type FieldOrAggregationReference = FieldReference | AggregationReference;
@@ -7,3 +8,22 @@ export type PivotSetting = {
rows: FieldReference[];
values: AggregationReference[];
};
+
+export interface LeftHeaderItem {
+ clicked: { value: string; column: Column };
+
+ isCollapsed: boolean;
+ hasChildren: boolean;
+ hasSubtotal?: boolean;
+ isSubtotal?: boolean;
+ isGrandTotal?: boolean;
+
+ depth: number;
+ maxDepthBelow: number;
+ offset: number;
+ span: number; // rows to span
+
+ path: string[];
+ rawValue: string;
+ value: string;
+}
diff --git a/frontend/src/metabase/visualizations/visualizations/PivotTable/utils.ts b/frontend/src/metabase/visualizations/visualizations/PivotTable/utils.ts
index 94e1af6630a..20e7dbf7eb5 100644
--- a/frontend/src/metabase/visualizations/visualizations/PivotTable/utils.ts
+++ b/frontend/src/metabase/visualizations/visualizations/PivotTable/utils.ts
@@ -6,7 +6,11 @@ import { measureText } from "metabase/lib/measure-text";
import type { Column } from "metabase-types/types/Dataset";
import type { Card } from "metabase-types/types/Card";
-import type { PivotSetting, FieldOrAggregationReference } from "./types";
+import type {
+ PivotSetting,
+ FieldOrAggregationReference,
+ LeftHeaderItem,
+} from "./types";
import { partitions } from "./partitions";
import {
@@ -15,6 +19,7 @@ import {
MIN_HEADER_CELL_WIDTH,
MAX_HEADER_CELL_WIDTH,
PIVOT_TABLE_FONT_SIZE,
+ MAX_ROWS_TO_MEASURE,
} from "./constants";
// adds or removes columns from the pivot settings based on the current query
@@ -88,25 +93,44 @@ export function isFormattablePivotColumn(column: Column) {
interface GetLeftHeaderWidthsProps {
rowIndexes: number[];
getColumnTitle: (columnIndex: number) => string;
+ leftHeaderItems?: LeftHeaderItem[];
fontFamily?: string;
}
export function getLeftHeaderWidths({
rowIndexes,
getColumnTitle,
+ leftHeaderItems = [],
fontFamily = "Lato",
}: GetLeftHeaderWidthsProps) {
- const widths = rowIndexes.map(rowIndex => {
+ const cellValues = getColumnValues(leftHeaderItems);
+
+ const widths = rowIndexes.map((rowIndex, depthIndex) => {
+ const computedHeaderWidth = Math.ceil(
+ measureText(getColumnTitle(rowIndex), {
+ weight: "bold",
+ family: fontFamily,
+ size: PIVOT_TABLE_FONT_SIZE,
+ }) + ROW_TOGGLE_ICON_WIDTH,
+ );
+
+ const computedCellWidth = Math.ceil(
+ Math.max(
+ // we need to use the depth index because the data is in depth order, not row index order
+ ...(cellValues[depthIndex]?.values?.map(
+ value =>
+ measureText(value, {
+ weight: "normal",
+ family: fontFamily,
+ size: PIVOT_TABLE_FONT_SIZE,
+ }) +
+ (cellValues[rowIndex]?.hasSubtotal ? ROW_TOGGLE_ICON_WIDTH : 0),
+ ) ?? [0]),
+ ),
+ );
+
const computedWidth =
- Math.ceil(
- measureText(getColumnTitle(rowIndex), {
- weight: "bold",
- family: fontFamily,
- size: PIVOT_TABLE_FONT_SIZE,
- }),
- ) +
- ROW_TOGGLE_ICON_WIDTH +
- CELL_PADDING;
+ Math.max(computedHeaderWidth, computedCellWidth) + CELL_PADDING;
if (computedWidth > MAX_HEADER_CELL_WIDTH) {
return MAX_HEADER_CELL_WIDTH;
@@ -123,3 +147,38 @@ export function getLeftHeaderWidths({
return { leftHeaderWidths: widths, totalHeaderWidths: total };
}
+
+type ColumnValueInfo = {
+ values: string[];
+ hasSubtotal: boolean;
+};
+
+export function getColumnValues(leftHeaderItems: LeftHeaderItem[]) {
+ const columnValues: ColumnValueInfo[] = [];
+
+ leftHeaderItems
+ .slice(0, MAX_ROWS_TO_MEASURE)
+ .forEach((leftHeaderItem: LeftHeaderItem) => {
+ const { value, depth, isSubtotal, isGrandTotal, hasSubtotal } =
+ leftHeaderItem;
+
+ // don't size based on subtotals or grand totals
+ if (!isSubtotal && !isGrandTotal) {
+ if (!columnValues[depth]) {
+ columnValues[depth] = {
+ values: [value],
+ hasSubtotal: false,
+ };
+ } else {
+ columnValues[depth].values.push(value);
+ }
+
+ // we need to track whether the column has a subtotal to size for the row expand icon
+ if (hasSubtotal) {
+ columnValues[depth].hasSubtotal = true;
+ }
+ }
+ });
+
+ return columnValues;
+}
diff --git a/frontend/src/metabase/visualizations/visualizations/PivotTable/utils.unit.spec.ts b/frontend/src/metabase/visualizations/visualizations/PivotTable/utils.unit.spec.ts
index 830fbaa2257..31d5fd65cc8 100644
--- a/frontend/src/metabase/visualizations/visualizations/PivotTable/utils.unit.spec.ts
+++ b/frontend/src/metabase/visualizations/visualizations/PivotTable/utils.unit.spec.ts
@@ -1,6 +1,7 @@
import type { Column } from "metabase-types/types/Dataset";
import type { Card } from "metabase-types/types/Card";
-import type { PivotSetting } from "./types";
+
+import type { PivotSetting, LeftHeaderItem } from "./types";
import {
isColumnValid,
@@ -8,9 +9,15 @@ import {
updateValueWithCurrentColumns,
addMissingCardBreakouts,
getLeftHeaderWidths,
+ getColumnValues,
} from "./utils";
-import { MAX_HEADER_CELL_WIDTH, MIN_HEADER_CELL_WIDTH } from "./constants";
+import {
+ MAX_HEADER_CELL_WIDTH,
+ MIN_HEADER_CELL_WIDTH,
+ CELL_PADDING,
+ ROW_TOGGLE_ICON_WIDTH,
+} from "./constants";
describe("Visualizations > Visualizations > PivotTable > utils", () => {
const cols = [
@@ -266,5 +273,114 @@ describe("Visualizations > Visualizations > PivotTable > utils", () => {
MAX_HEADER_CELL_WIDTH,
]);
});
+
+ it("should return the wider of the column header or data width", () => {
+ const data = [
+ { depth: 0, value: "x".repeat(150) },
+ { depth: 0, value: "foo2" },
+ { depth: 1, value: "bar1" },
+ { depth: 1, value: "bar2" },
+ { depth: 2, value: "baz1" },
+ { depth: 4, value: "boo1" },
+ ] as LeftHeaderItem[];
+
+ const { leftHeaderWidths } = getLeftHeaderWidths({
+ rowIndexes: [0, 1, 2, 3, 4],
+ leftHeaderItems: data,
+ getColumnTitle: () => "x".repeat(70),
+ });
+
+ expect(leftHeaderWidths).toEqual([
+ 150 + CELL_PADDING,
+ 70 + CELL_PADDING + ROW_TOGGLE_ICON_WIDTH,
+ 70 + CELL_PADDING + ROW_TOGGLE_ICON_WIDTH,
+ 70 + CELL_PADDING + ROW_TOGGLE_ICON_WIDTH,
+ 70 + CELL_PADDING + ROW_TOGGLE_ICON_WIDTH,
+ ]);
+ });
+
+ it("should factor in the toggle icon width for columns with subtotals", () => {
+ const data = [
+ { depth: 0, value: "x".repeat(100), hasSubtotal: true },
+ { depth: 0, value: "foo2" },
+ { depth: 1, value: "bar1" },
+ { depth: 1, value: "bar2" },
+ { depth: 2, value: "baz1" },
+ { depth: 4, value: "boo1" },
+ ] as LeftHeaderItem[];
+
+ const { leftHeaderWidths } = getLeftHeaderWidths({
+ rowIndexes: [0, 1, 2, 3, 4],
+ leftHeaderItems: data,
+ getColumnTitle: () => "test-123",
+ });
+
+ expect(leftHeaderWidths).toEqual([
+ 100 + CELL_PADDING + ROW_TOGGLE_ICON_WIDTH,
+ MIN_HEADER_CELL_WIDTH,
+ MIN_HEADER_CELL_WIDTH,
+ MIN_HEADER_CELL_WIDTH,
+ MIN_HEADER_CELL_WIDTH,
+ ]);
+ });
+ });
+
+ describe("getColumnValues", () => {
+ it("can collect column values from left header data", () => {
+ const data = [
+ { depth: 0, value: "foo1" },
+ { depth: 0, value: "foo2" },
+ { depth: 1, value: "bar1" },
+ { depth: 1, value: "bar2" },
+ { depth: 2, value: "baz1" },
+ { depth: 4, value: "boo1" },
+ ] as LeftHeaderItem[];
+
+ const result = getColumnValues(data);
+
+ expect(result).toEqual([
+ { values: ["foo1", "foo2"], hasSubtotal: false },
+ { values: ["bar1", "bar2"], hasSubtotal: false },
+ { values: ["baz1"], hasSubtotal: false },
+ undefined, // no depth of 3
+ { values: ["boo1"], hasSubtotal: false },
+ ]);
+ });
+
+ it("detects columns with subtotals", () => {
+ const data = [
+ { depth: 0, value: "foo1", hasSubtotal: false },
+ { depth: 0, value: "foo2", hasSubtotal: true },
+ { depth: 1, value: "bar1", hasSubtotal: false },
+ { depth: 1, value: "bar2", hasSubtotal: false },
+ { depth: 2, value: "baz1", hasSubtotal: true },
+ ] as LeftHeaderItem[];
+
+ const result = getColumnValues(data);
+
+ expect(result).toEqual([
+ { values: ["foo1", "foo2"], hasSubtotal: true },
+ { values: ["bar1", "bar2"], hasSubtotal: false },
+ { values: ["baz1"], hasSubtotal: true },
+ ]);
+ });
+
+ it("handles null values", () => {
+ const data = [
+ { depth: 0, value: "foo1", hasSubtotal: false },
+ { depth: 0, value: null, hasSubtotal: true },
+ { depth: 1, value: "bar1", hasSubtotal: false },
+ { depth: 1, value: "bar2", hasSubtotal: false },
+ { depth: 2, value: "baz1", hasSubtotal: true },
+ ] as LeftHeaderItem[];
+
+ const result = getColumnValues(data);
+
+ expect(result).toEqual([
+ { values: ["foo1", null], hasSubtotal: true },
+ { values: ["bar1", "bar2"], hasSubtotal: false },
+ { values: ["baz1"], hasSubtotal: true },
+ ]);
+ });
});
});
--
GitLab