From 873498cdaf6c2fa1e9e4e08af64e45c055d5577f Mon Sep 17 00:00:00 2001 From: "Adrien Minne (adrm)" Date: Mon, 18 Nov 2024 15:07:58 +0100 Subject: [PATCH] [FIX] chart: trend line legend is wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The trend line legend is a circle instead of a line in scatter charts. And it's a rectangle in bar chart. Also the onClick of the legend was slightly wrong: we used the index of the legend item to check which dataset to hide. Which didn't work if for whatever reason ChartJs decided to change the order of the legend items. closes odoo/o-spreadsheet#5226 Task: 4342471 Signed-off-by: Lucas Lefèvre (lul) --- .../figures/charts/runtime/chartjs_legend.ts | 49 ++++++++++++------- tests/figures/chart/bar_chart_plugin.test.ts | 26 ++++++++++ .../figures/chart/combo_chart_plugin.test.ts | 2 + tests/figures/chart/line_chart_plugin.test.ts | 2 + .../figures/chart/radar_chart_plugin.test.ts | 2 + 5 files changed, 64 insertions(+), 17 deletions(-) diff --git a/src/helpers/figures/charts/runtime/chartjs_legend.ts b/src/helpers/figures/charts/runtime/chartjs_legend.ts index 7891e13506..c85b5f5340 100644 --- a/src/helpers/figures/charts/runtime/chartjs_legend.ts +++ b/src/helpers/figures/charts/runtime/chartjs_legend.ts @@ -17,7 +17,7 @@ import { import { ComboChartDefinition } from "../../../../types/chart/combo_chart"; import { RadarChartDefinition } from "../../../../types/chart/radar_chart"; import { ColorGenerator } from "../../../color"; -import { chartFontColor, getPieColors } from "../chart_common"; +import { TREND_LINE_XAXIS_ID, chartFontColor, getPieColors } from "../chart_common"; type ChartLegend = DeepPartial>; @@ -96,11 +96,12 @@ export function getScatterChartLegend( return { ...INTERACTIVE_LEGEND_CONFIG, ...getLegendDisplayOptions(definition, args), - labels: { - color: chartFontColor(definition.background), - boxHeight: 6, - usePointStyle: true, - }, + ...getCustomLegendLabels(chartFontColor(definition.background), { + pointStyle: "circle", + // the stroke is the border around the circle, so increasing its size with the chart's color reduce the size of the circle + strokeStyle: definition.background || "#ffffff", + lineWidth: 8, + }), }; } @@ -203,10 +204,10 @@ export const INTERACTIVE_LEGEND_CONFIG = { target.style.cursor = "default"; }, onClick: (event, legendItem, legend) => { - if (!legend.legendItems) { + const index = legendItem.datasetIndex; + if (!legend.legendItems || index === undefined) { return; } - const index = legend.legendItems.indexOf(legendItem); if (legend.chart.isDatasetVisible(index)) { legend.chart.hide(index); } else { @@ -232,15 +233,29 @@ function getCustomLegendLabels( color: fontColor, usePointStyle: true, generateLabels: (chart: Chart) => - chart.data.datasets.map((dataset, index) => ({ - text: dataset.label ?? "", - fontColor, - strokeStyle: dataset.borderColor as Color, - fillStyle: dataset.backgroundColor as Color, - hidden: !chart.isDatasetVisible(index), - pointStyle: dataset.type === "line" ? "line" : "rect", - ...legendLabelConfig, - })), + chart.data.datasets.map((dataset, index) => { + if (dataset["xAxisID"] === TREND_LINE_XAXIS_ID) { + return { + text: dataset.label ?? "", + fontColor, + strokeStyle: dataset.borderColor as Color, + hidden: !chart.isDatasetVisible(index), + pointStyle: "line", + datasetIndex: index, + lineWidth: 3, + }; + } + return { + text: dataset.label ?? "", + fontColor, + strokeStyle: dataset.borderColor as Color, + fillStyle: dataset.backgroundColor as Color, + hidden: !chart.isDatasetVisible(index), + pointStyle: dataset.type === "line" ? "line" : "rect", + datasetIndex: index, + ...legendLabelConfig, + }; + }), }, }; } diff --git a/tests/figures/chart/bar_chart_plugin.test.ts b/tests/figures/chart/bar_chart_plugin.test.ts index 869a864512..f784647634 100644 --- a/tests/figures/chart/bar_chart_plugin.test.ts +++ b/tests/figures/chart/bar_chart_plugin.test.ts @@ -163,6 +163,7 @@ describe("bar chart", () => { lineWidth: 3, pointStyle: "rect", strokeStyle: "#FFFFFF", + datasetIndex: 0, }, { fontColor: "#000000", @@ -172,6 +173,7 @@ describe("bar chart", () => { lineWidth: 3, pointStyle: "rect", strokeStyle: "#FFFFFF", + datasetIndex: 1, }, ]); }); @@ -225,4 +227,28 @@ describe("bar chart", () => { runtime = model.getters.getChartRuntime("chartId") as BarChartRuntime; expect(runtime.chartJsConfig.data.datasets[0].borderColor).toBe("#f00"); }); + + test("Bar chart trend line legend", () => { + const model = createModelFromGrid({ A1: "1", A2: "2" }); + createChart( + model, + { + dataSets: [ + { + dataRange: "Sheet1!A1:A2", + backgroundColor: "#f00", + label: "serie_1", + trend: { type: "polynomial", order: 1, color: "#f0f", display: true }, + }, + ], + dataSetsHaveTitle: false, + type: "bar", + }, + "1" + ); + expect(getChartLegendLabels(model, "1")).toMatchObject([ + { text: "serie_1", fillStyle: "#f00", pointStyle: "rect" }, + { text: "Trend line for serie_1", strokeStyle: "#f0f", pointStyle: "line" }, + ]); + }); }); diff --git a/tests/figures/chart/combo_chart_plugin.test.ts b/tests/figures/chart/combo_chart_plugin.test.ts index 523b9c565a..cf865430e7 100644 --- a/tests/figures/chart/combo_chart_plugin.test.ts +++ b/tests/figures/chart/combo_chart_plugin.test.ts @@ -134,6 +134,7 @@ describe("combo chart", () => { pointStyle: "rect", strokeStyle: "#f00", fillStyle: "#f00", + datasetIndex: 0, }, { fontColor: "#000000", @@ -143,6 +144,7 @@ describe("combo chart", () => { pointStyle: "line", strokeStyle: "#00f", fillStyle: "#00f", + datasetIndex: 1, }, ]); }); diff --git a/tests/figures/chart/line_chart_plugin.test.ts b/tests/figures/chart/line_chart_plugin.test.ts index 42b7dddbb4..ed6431db41 100644 --- a/tests/figures/chart/line_chart_plugin.test.ts +++ b/tests/figures/chart/line_chart_plugin.test.ts @@ -197,6 +197,7 @@ describe("line chart", () => { lineWidth: 3, pointStyle: "line", strokeStyle: "#f00", + datasetIndex: 0, }, { fontColor: "#000000", @@ -206,6 +207,7 @@ describe("line chart", () => { lineWidth: 3, pointStyle: "line", strokeStyle: "#00f", + datasetIndex: 1, }, ]); }); diff --git a/tests/figures/chart/radar_chart_plugin.test.ts b/tests/figures/chart/radar_chart_plugin.test.ts index e05bd1718e..6026cd70bb 100644 --- a/tests/figures/chart/radar_chart_plugin.test.ts +++ b/tests/figures/chart/radar_chart_plugin.test.ts @@ -88,6 +88,7 @@ describe("radar chart", () => { lineWidth: 3, pointStyle: "line", strokeStyle: "#f00", + datasetIndex: 0, }, { fontColor: "#000000", @@ -97,6 +98,7 @@ describe("radar chart", () => { lineWidth: 3, pointStyle: "line", strokeStyle: "#00f", + datasetIndex: 1, }, ]); });