[IMP] charts: disable legend for single dataset

Task Description When drawing a chart with only one dataset, we are allowed to choose the position of the legend but no legend will appears even after changing the position. This commit aims to fix thie issue, disabling the select box allowing to change the legend position. The chart padding has also been adapted to have more padding when there is no title/legend. Related Task closes #4600 Task: 4028903 Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
odoo · Jul 29, 2024 · c80fc42 · c80fc42
1 parent f16b623
commit c80fc42
Show file tree

Hide file tree

Showing 6 changed files with 137 additions and 24 deletions.
diff --git a/src/helpers/figures/charts/bar_chart.ts b/src/helpers/figures/charts/bar_chart.ts
@@ -35,6 +35,7 @@ import {
   chartFontColor,
   checkDataset,
   checkLabelRange,
+  computeChartPadding,
   copyDataSetsWithNewSheetId,
   copyLabelRangeWithNewSheetId,
   createDataSets,
@@ -236,14 +237,17 @@ function getBarConfiguration(
   const legend: DeepPartial<LegendOptions<"bar">> = {
     labels: { color: fontColor },
   };
-  if ((!chart.labelRange && chart.dataSets.length === 1) || chart.legendPosition === "none") {
+  if (chart.legendPosition === "none") {
     legend.display = false;
   } else {
     legend.position = chart.legendPosition;
   }
   config.options.plugins!.legend = { ...config.options.plugins?.legend, ...legend };
   config.options.layout = {
-    padding: { left: 20, right: 20, top: chart.title ? 10 : 25, bottom: 10 },
+    padding: computeChartPadding({
+      displayTitle: !!chart.title.text,
+      displayLegend: chart.legendPosition === "top",
+    }),
   };
   config.options.indexAxis = chart.horizontal ? "y" : "x";
 

diff --git a/src/helpers/figures/charts/chart_common.ts b/src/helpers/figures/charts/chart_common.ts
@@ -428,3 +428,24 @@ export function getDefinedAxis(definition: ChartWithAxisDefinition): {
   useLeftAxis ||= !useRightAxis;
   return { useLeftAxis, useRightAxis };
 }
+
+export function computeChartPadding({
+  displayTitle,
+  displayLegend,
+}: {
+  displayTitle: boolean;
+  displayLegend: boolean;
+}): {
+  top: number;
+  bottom: number;
+  left: number;
+  right: number;
+} {
+  let top = 25;
+  if (displayTitle) {
+    top = 0;
+  } else if (displayLegend) {
+    top = 10;
+  }
+  return { left: 20, right: 20, top, bottom: 10 };
+}
diff --git a/src/helpers/figures/charts/chart_common_line_scatter.ts b/src/helpers/figures/charts/chart_common_line_scatter.ts
@@ -7,7 +7,12 @@ import { getChartTimeOptions, timeFormatLuxonCompatible } from "../../chart_date
 import { ColorGenerator, colorToRGBA, rgbaToHex } from "../../color";
 import { formatValue } from "../../format";
 import { deepCopy, findNextDefinedValue } from "../../misc";
-import { chartFontColor, getChartAxisTitleRuntime, getDefinedAxis } from "./chart_common";
+import {
+  chartFontColor,
+  computeChartPadding,
+  getChartAxisTitleRuntime,
+  getDefinedAxis,
+} from "./chart_common";
 import {
   aggregateDataForLabels,
   filterEmptyDataPoints,
@@ -131,14 +136,17 @@ function getLineOrScatterConfiguration(
       },
     },
   };
-  if ((!chart.labelRange && chart.dataSets.length === 1) || chart.legendPosition === "none") {
+  if (chart.legendPosition === "none") {
     legend.display = false;
   } else {
     legend.position = chart.legendPosition;
   }
   Object.assign(config.options.plugins!.legend || {}, legend);
   config.options.layout = {
-    padding: { left: 20, right: 20, top: chart.title ? 10 : 25, bottom: 10 },
+    padding: computeChartPadding({
+      displayTitle: !!chart.title.text,
+      displayLegend: chart.legendPosition === "top",
+    }),
   };
 
   config.options.scales = {

diff --git a/src/helpers/figures/charts/combo_chart.ts b/src/helpers/figures/charts/combo_chart.ts
@@ -35,6 +35,7 @@ import {
   chartFontColor,
   checkDataset,
   checkLabelRange,
+  computeChartPadding,
   copyDataSetsWithNewSheetId,
   copyLabelRangeWithNewSheetId,
   createDataSets,
@@ -249,14 +250,17 @@ export function createComboChartRuntime(chart: ComboChart, getters: Getters): Co
     labels: { color: fontColor },
     reverse: true,
   };
-  if ((!chart.labelRange && chart.dataSets.length === 1) || chart.legendPosition === "none") {
+  if (chart.legendPosition === "none") {
     legend.display = false;
   } else {
     legend.position = chart.legendPosition;
   }
   config.options.plugins!.legend = { ...config.options.plugins?.legend, ...legend };
   config.options.layout = {
-    padding: { left: 20, right: 20, top: chart.title ? 10 : 25, bottom: 10 },
+    padding: computeChartPadding({
+      displayTitle: !!chart.title.text,
+      displayLegend: chart.legendPosition === "top",
+    }),
   };
 
   config.options.scales = {

diff --git a/tests/figures/chart/__snapshots__/chart_plugin.test.ts.snap b/tests/figures/chart/__snapshots__/chart_plugin.test.ts.snap
@@ -50,7 +50,7 @@ exports[`Linear/Time charts font color is white with a dark background color 1`]
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -195,7 +195,7 @@ exports[`Linear/Time charts snapshot test of chartJS configuration for date char
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -348,7 +348,7 @@ exports[`Linear/Time charts snapshot test of chartJS configuration for linear ch
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -475,7 +475,7 @@ exports[`datasource tests create a chart with stacked bar 1`] = `
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -576,7 +576,7 @@ exports[`datasource tests create chart with a dataset of one cell (no title) 1`]
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -710,7 +710,7 @@ exports[`datasource tests create chart with column datasets 1`] = `
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -858,7 +858,7 @@ exports[`datasource tests create chart with column datasets with category title
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -1007,7 +1007,7 @@ exports[`datasource tests create chart with column datasets without series title
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -1140,7 +1140,7 @@ exports[`datasource tests create chart with only the dataset title (no data) 1`]
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -1278,7 +1278,7 @@ exports[`datasource tests create chart with rectangle dataset 1`] = `
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -1426,7 +1426,7 @@ exports[`datasource tests create chart with row datasets 1`] = `
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -1574,7 +1574,7 @@ exports[`datasource tests create chart with row datasets with category title 1`]
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,
@@ -1723,7 +1723,7 @@ exports[`datasource tests create chart with row datasets without series title 1`
           "bottom": 10,
           "left": 20,
           "right": 20,
-          "top": 10,
+          "top": 0,
         },
       },
       "maintainAspectRatio": false,

diff --git a/tests/figures/chart/chart_plugin.test.ts b/tests/figures/chart/chart_plugin.test.ts
@@ -1555,19 +1555,19 @@ describe("Chart without labels", () => {
     aggregated: false,
   };
 
-  test("The legend is not displayed when there is only one dataSet and no label", () => {
+  test("The legend is displayed even when there is only one dataSet or no label", () => {
     createChart(model, defaultChart, "42");
-    expect(getChartConfiguration(model, "42").options?.plugins?.legend?.display).toBe(false);
+    expect(getChartConfiguration(model, "42").options?.plugins?.legend?.position).toBe("top");
 
     createChart(
       model,
       { ...defaultChart, dataSets: [{ dataRange: "A1:A2" }, { dataRange: "A3:A4" }] },
       "43"
     );
-    expect(getChartConfiguration(model, "43").options?.plugins?.legend?.display).toBeUndefined();
+    expect(getChartConfiguration(model, "42").options?.plugins?.legend?.position).toBe("top");
 
     createChart(model, { ...defaultChart, labelRange: "B1:B2" }, "44");
-    expect(getChartConfiguration(model, "44").options?.plugins?.legend?.display).toBeUndefined();
+    expect(getChartConfiguration(model, "42").options?.plugins?.legend?.position).toBe("top");
   });
 
   test("Labels are empty if there is only one dataSet and no label", () => {
@@ -1913,6 +1913,82 @@ describe("Chart design configuration", () => {
       const label = getTooltipLabel(runtime, 0, 0);
       expect(label).toEqual("6,000");
     });
+
+    test.each(["line", "scatter", "combo", "bar"] as const)(
+      "%s chart with no title and no legend have the correct padding",
+      (chartType) => {
+        createChart(
+          model,
+          {
+            dataSets: [{ dataRange: "B1:B2" }],
+            labelRange: "A1:A2",
+            type: chartType,
+            legendPosition: "none",
+            title: { text: "" },
+          },
+          "1"
+        );
+        const config = getChartConfiguration(model, "1");
+        expect(config.options.layout.padding).toEqual({
+          top: 25,
+          bottom: 10,
+          left: 20,
+          right: 20,
+        });
+      }
+    );
+
+    test.each(["line", "scatter", "combo", "bar"] as const)(
+      "%s chart with no title but a legend have the correct padding",
+      (chartType) => {
+        createChart(
+          model,
+          {
+            dataSets: [{ dataRange: "B1:B2" }],
+            labelRange: "A1:A2",
+            type: chartType,
+            legendPosition: "bottom",
+            title: { text: "" },
+          },
+          "1"
+        );
+        let config = getChartConfiguration(model, "1");
+        expect(config.options.layout.padding).toEqual({
+          top: 25,
+          bottom: 10,
+          left: 20,
+          right: 20,
+        });
+
+        updateChart(model, "1", { legendPosition: "top" });
+        config = getChartConfiguration(model, "1");
+        expect(config.options.layout.padding.top).toEqual(10);
+      }
+    );
+
+    test.each(["line", "scatter", "combo", "bar"] as const)(
+      "%s chart with a title and a legend have the correct padding",
+      (chartType) => {
+        createChart(
+          model,
+          {
+            dataSets: [{ dataRange: "B1:B2" }],
+            labelRange: "A1:A2",
+            type: chartType,
+            legendPosition: "bottom",
+            title: { text: "test" },
+          },
+          "1"
+        );
+        const config = getChartConfiguration(model, "1");
+        expect(config.options.layout.padding).toEqual({
+          top: 0,
+          bottom: 10,
+          left: 20,
+          right: 20,
+        });
+      }
+    );
   });
 
   describe("Pie Chart tooltip", () => {