Skip to content

Commit

Permalink
[FIX] charts: time axis broken with long formatted date
Browse files Browse the repository at this point in the history
In the charts, we truncate the labels if they are too long
(>20 characters)[1]. But in line chart with time axis, chartJS parses
the labels to get a date. This parsing obviously fails if the label
is truncated.

This commit fixes the issue by not truncating the labels if the
axis is a time axis.

[1] e.g. a format like "12/04/2024 12:34:56 AM".

closes #4133

Task: 3857242
X-original-commit: 1c949f2
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
Signed-off-by: Adrien Minne (adrm) <adrm@odoo.com>
  • Loading branch information
hokolomopo committed Apr 26, 2024
1 parent 065ac98 commit 1f97287
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 8 deletions.
4 changes: 2 additions & 2 deletions src/helpers/figures/charts/chart_ui_common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export function getDefaultChartJsRuntime(
chart: AbstractChart,
labels: string[],
fontColor: Color,
{ format, locale }: LocaleFormat
{ format, locale, truncateLabels }: LocaleFormat & { truncateLabels?: boolean }
): Required<ChartConfiguration> {
const options: ChartOptions = {
// https://www.chartjs.org/docs/latest/general/responsive.html
Expand Down Expand Up @@ -148,7 +148,7 @@ export function getDefaultChartJsRuntime(
type: chart.type as ChartType,
options,
data: {
labels: labels.map(truncateLabel),
labels: truncateLabels ? labels.map(truncateLabel) : labels,
datasets: [],
},
plugins: [],
Expand Down
11 changes: 6 additions & 5 deletions src/helpers/figures/charts/line_chart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -289,10 +289,10 @@ function canBeLinearChart(labelRange: Range | undefined, getters: Getters): bool
function getLineConfiguration(
chart: LineChart,
labels: string[],
localeFormat: LocaleFormat
options: LocaleFormat & { truncateLabels?: boolean }
): Required<ChartConfiguration> {
const fontColor = chartFontColor(chart.background);
const config = getDefaultChartJsRuntime(chart, labels, fontColor, localeFormat);
const config = getDefaultChartJsRuntime(chart, labels, fontColor, options);

const legend: DeepPartial<LegendOptions<"line">> = {
labels: {
Expand Down Expand Up @@ -335,7 +335,7 @@ function getLineConfiguration(
callback: (value) => {
value = Number(value);
if (isNaN(value)) return value;
const { locale, format } = localeFormat;
const { locale, format } = options;
return formatValue(value, {
locale,
format: !format && Math.abs(value) >= 1000 ? "#,##" : format,
Expand Down Expand Up @@ -373,9 +373,10 @@ export function createLineChartRuntime(chart: LineChart, getters: Getters): Line
}

const locale = getters.getLocale();
const truncateLabels = axisType === "category";
const dataSetFormat = getChartDatasetFormat(getters, chart.dataSets);
const localeFormat = { format: dataSetFormat, locale };
const config = getLineConfiguration(chart, labels, localeFormat);
const options = { format: dataSetFormat, locale, truncateLabels };
const config = getLineConfiguration(chart, labels, options);
const labelFormat = getChartLabelFormat(getters, chart.labelRange)!;
if (axisType === "time") {
const axis = {
Expand Down
30 changes: 29 additions & 1 deletion tests/figures/chart/chart_plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,11 @@ import {
import { getPlugin, mockChart, nextTick, target } from "../../test_helpers/helpers";

import { ChartTerms } from "../../../src/components/translations_terms";
import { FIGURE_ID_SPLITTER } from "../../../src/constants";
import { FIGURE_ID_SPLITTER, MAX_CHAR_LABEL } from "../../../src/constants";
import { BarChart } from "../../../src/helpers/figures/charts";
import { ChartPlugin } from "../../../src/plugins/core";
import { FR_LOCALE } from "../../test_helpers/constants";
import { getCellContent } from "../../test_helpers/getters_helpers";

jest.mock("../../../src/helpers/uuid", () => require("../../__mocks__/uuid"));

Expand Down Expand Up @@ -1976,6 +1977,33 @@ describe("Linear/Time charts", () => {
expect(chart.data!.datasets![0].data![1]).toEqual({ y: undefined, x: "1/17/1900" });
});

test("date chart: long labels are not truncated", () => {
model.dispatch("SET_FORMATTING", {
sheetId: model.getters.getActiveSheetId(),
target: [toZone("C2")],
format: "m/d/yyyy hh:mm:ss a",
});
setCellContent(model, "C2", "300");
setCellContent(model, "B2", "10");
const formattedValue = getCellContent(model, "C2");
expect(formattedValue).toEqual("10/26/1900 12:00:00 AM");
expect(formattedValue.length).toBeGreaterThan(MAX_CHAR_LABEL);
createChart(
model,
{
type: "line",
dataSets: ["B2"],
labelRange: "C2",
labelsAsText: false,
dataSetsHaveTitle: false,
},
chartId
);
const chart = (model.getters.getChartRuntime(chartId) as LineChartRuntime).chartJsConfig;
expect(chart.data!.labels![0]).toEqual(formattedValue);
expect(chart.data!.datasets![0].data![0]).toEqual({ y: 10, x: formattedValue });
});

test("linear chart: label 0 isn't set to undefined", () => {
setCellContent(model, "B2", "0");
setCellContent(model, "B3", "1");
Expand Down

0 comments on commit 1f97287

Please sign in to comment.