Fix pie chart legend issues (#50116)

* remove LegendVertical logic that preserves legend width

* memoize pie legend items

* prevent pie from jumping when toggle legend

* fix responsive legend logic, cleanup

* drop hyphens from the pie legend (#29597)

* eliminate flickering on render due to not having dimensions

* shortcut

* specs

* test vertical layout has no hyphens

* setup func

* set explicit showLegend=true default

frontend/src/metabase/visualizations/components/ChartWithLegend.jsx

@@ -14,11 +14,14 @@ const GRID_ASPECT_RATIO = 4 / 3;
 const PADDING = 14;
 
 const DEFAULT_GRID_SIZE = 100;
+export const HIDE_HORIZONTAL_LEGEND_THRESHOLD = 180;
+export const HIDE_SECONDARY_INFO_THRESHOLD = 260;
 
 class ChartWithLegend extends Component {
   static defaultProps = {
     aspectRatio: 1,
     style: {},
+    showLegend: true,
   };
 
   render() {
@@ -57,16 +60,13 @@ class ChartWithLegend extends Component {
     let type;
     let LegendComponent;
     const isHorizontal = gridSize.width > gridSize.height / GRID_ASPECT_RATIO;
-    if (showLegend === false) {
+    if (!showLegend) {
       type = "small";
-    } else if (
-      !gridSize ||
-      (isHorizontal &&
-        (showLegend || gridSize.width > 4 || gridSize.height > 4))
-    ) {
+    } else if (isHorizontal && width > HIDE_HORIZONTAL_LEGEND_THRESHOLD) {
       type = "horizontal";
       LegendComponent = LegendVertical;
-      if (gridSize && gridSize.width < 6) {
+
+      if (width < HIDE_SECONDARY_INFO_THRESHOLD) {
         legendTitles = legendTitles.map(title =>
           Array.isArray(title) ? title.slice(0, 1) : title,
         );
@@ -78,18 +78,14 @@ class ChartWithLegend extends Component {
         chartWidth = desiredWidth;
       }
       chartHeight = height;
-    } else if (
-      !isHorizontal &&
-      (showLegend || (gridSize.height > 3 && gridSize.width > 2))
-    ) {
+    } else if (!isHorizontal && gridSize.height > 3 && gridSize.width > 2) {
       type = "vertical";
       LegendComponent = LegendHorizontal;
       legendTitles = legendTitles.map(title =>
-        Array.isArray(title) ? title.join(" – ") : title,
+        Array.isArray(title) ? title.join(" ") : title,
       );
       const desiredHeight = width * (1 / aspectRatio);
       if (desiredHeight > height * (3 / 4)) {
-        // chartHeight = height * (3 / 4);
         flexChart = true;
       } else {
         chartHeight = desiredHeight;
@@ -99,6 +95,8 @@ class ChartWithLegend extends Component {
       type = "small";
     }
 
+    const hasDimensions = width > 0 && height > 0;
+
     const legend = LegendComponent ? (
       <LegendComponent
         className={styles.Legend}
@@ -138,7 +136,7 @@ class ChartWithLegend extends Component {
           className={cx(styles.Chart)}
           style={{ width: chartWidth, height: chartHeight }}
         >
-          {children}
+          {hasDimensions ? children : null}
         </div>
         {/* spacer div to balance legend */}
         {legend && (

frontend/src/metabase/visualizations/components/ChartWithLegend.unit.spec.jsx

+import { render, screen } from "@testing-library/react";
+
+import ChartWithLegend, {
+  HIDE_HORIZONTAL_LEGEND_THRESHOLD,
+  HIDE_SECONDARY_INFO_THRESHOLD,
+} from "./ChartWithLegend";
+
+const defaultProps = {
+  legendTitles: [
+    ["Series 1", "50%"],
+    ["Series 2", "50%"],
+  ],
+  legendColors: ["Red", "Green"],
+  showLegend: true,
+  width: 800,
+  height: 600,
+  gridSize: { width: 8, height: 6 },
+  children: <div>Chart stub</div>,
+};
+
+const setup = (props = {}) => {
+  render(<ChartWithLegend {...defaultProps} {...props} />);
+};
+
+describe("ChartWithLegend", () => {
+  it("should show full legend titles when width is above secondary info threshold", () => {
+    setup();
+
+    expect(screen.getByTestId("chart-legend")).toHaveTextContent("Series 1");
+    expect(screen.getByTestId("chart-legend")).toHaveTextContent("Series 2");
+    expect(screen.getByTestId("chart-legend")).toHaveTextContent("50%");
+  });
+
+  it("should hide secondary info when width is below threshold", () => {
+    setup({ width: HIDE_SECONDARY_INFO_THRESHOLD - 1 });
+
+    expect(screen.getByTestId("chart-legend")).toHaveTextContent("Series 1");
+    expect(screen.getByTestId("chart-legend")).toHaveTextContent("Series 2");
+    expect(screen.getByTestId("chart-legend")).not.toHaveTextContent("50%");
+  });
+
+  it("should not render legend when width is below horizontal legend threshold", () => {
+    setup({ width: HIDE_HORIZONTAL_LEGEND_THRESHOLD - 1 });
+
+    expect(screen.queryByTestId("chart-legend")).not.toBeInTheDocument();
+  });
+
+  it("should not render chart content when dimensions are zero", () => {
+    setup({ width: 0, height: 0 });
+
+    expect(screen.queryByText("Chart stub")).not.toBeInTheDocument();
+  });
+
+  it("should render chart content when dimensions are valid", () => {
+    setup();
+
+    expect(screen.getByText("Chart stub")).toBeInTheDocument();
+  });
+
+  it("should join legend titles with space in vertical layout", () => {
+    setup({
+      width: 400,
+      height: 600,
+      gridSize: { width: 3, height: 6 },
+      aspectRatio: 0.5,
+      legendTitles: [
+        ["Foo", "10%"],
+        ["Bar", "90%"],
+      ],
+    });
+
+    expect(screen.getByTestId("chart-legend")).toHaveTextContent("Foo 10%");
+    expect(screen.getByTestId("chart-legend")).toHaveTextContent("Bar 90%");
+  });
+});

frontend/src/metabase/visualizations/components/EChartsRenderer/EChartsRenderer.styled.tsx

 import styled from "@emotion/styled";
 
 export const EChartsRendererRoot = styled.div`
+  display: flex;
+  align-items: center;
+  justify-content: center;
   /* HACK: zrender adds user-select: none to the root svg element which prevents users from selecting text on charts */
   & svg {
     user-select: auto !important;

frontend/src/metabase/visualizations/components/LegendVertical.jsx

@@ -18,16 +18,10 @@ export default class LegendVertical extends Component {
   state = {
     overflowCount: 0,
     size: null,
-    listWidth: null,
   };
 
   listRef = createRef();
 
-  componentDidMount() {
-    const listWidth = this.listRef.current.getBoundingClientRect().width;
-    this.setState({ listWidth });
-  }
-
   componentDidUpdate(prevProps, prevState) {
     // Get the bounding rectangle of the chart widget to determine if
     // legend items will overflow the widget area
@@ -55,20 +49,6 @@ export default class LegendVertical extends Component {
         this.setState({ overflowCount, size });
       }
     }
-
-    const [previousSampleTitle] = prevProps.titles || [];
-    const [sampleTitle] = this.props.titles || [];
-
-    if (
-      sampleTitle &&
-      previousSampleTitle &&
-      previousSampleTitle.length !== sampleTitle.length
-    ) {
-      this.setState({ listWidth: null }, () => {
-        const listWidth = this.listRef.current.getBoundingClientRect().width;
-        this.setState({ listWidth });
-      });
-    }
   }
 
   render() {
@@ -95,7 +75,6 @@ export default class LegendVertical extends Component {
     return (
       <ol
         className={cx(className, LegendS.Legend, LegendS.vertical)}
-        style={{ width: this.state.listWidth }}
         ref={this.listRef}
       >
         {items.map((title, index) => {

frontend/src/metabase/visualizations/visualizations/PieChart/PieChart.tsx

@@ -117,27 +117,34 @@ export function PieChart(props: VisualizationProps) {
 
   const eventHandlers = useChartEvents(props, chartRef, chartModel);
 
-  const slices = getArrayFromMapValues(chartModel.sliceTree);
-  const legendTitles = slices
-    .filter(s => s.includeInLegend)
-    .map(s => {
-      const label = s.name;
-
-      // Hidden slices don't have a percentage
-      const sliceHidden = s.normalizedPercentage === 0;
-      const percentDisabled =
-        settings["pie.percent_visibility"] !== "legend" &&
-        settings["pie.percent_visibility"] !== "both";
-
-      if (sliceHidden || percentDisabled) {
-        return [label];
-      }
-
-      return [
-        label,
-        formatters.formatPercent(s.normalizedPercentage, "legend"),
-      ];
-    });
+  const slices = useMemo(
+    () => getArrayFromMapValues(chartModel.sliceTree),
+    [chartModel.sliceTree],
+  );
+  const legendTitles = useMemo(
+    () =>
+      slices
+        .filter(s => s.includeInLegend)
+        .map(s => {
+          const label = s.name;
+
+          // Hidden slices don't have a percentage
+          const sliceHidden = s.normalizedPercentage === 0;
+          const percentDisabled =
+            settings["pie.percent_visibility"] !== "legend" &&
+            settings["pie.percent_visibility"] !== "both";
+
+          if (sliceHidden || percentDisabled) {
+            return [label];
+          }
+
+          return [
+            label,
+            formatters.formatPercent(s.normalizedPercentage, "legend"),
+          ];
+        }),
+    [formatters, settings, slices],
+  );
 
   const hiddenSlicesLegendIndices = slices
     .filter(s => s.includeInLegend)
@@ -157,7 +164,7 @@ export function PieChart(props: VisualizationProps) {
     );
 
   const handleToggleSeriesVisibility = (
-    event: MouseEvent,
+    _event: MouseEvent,
     sliceIndex: number,
   ) => {
     const slice = slices[sliceIndex];