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 #4153

Task: 3857242
X-original-commit: 0ff6dfc
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 29, 2024
1 parent b9c7a12 commit 64d67ce
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 9 deletions.
11 changes: 6 additions & 5 deletions src/helpers/figures/charts/chart_common_line_scatter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,10 @@ function isLuxonTimeAdapterInstalled() {
function getLineOrScatterConfiguration(
chart: LineChart | ScatterChart,
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 @@ -160,7 +160,7 @@ function getLineOrScatterConfiguration(
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 @@ -208,9 +208,10 @@ export function createLineOrScatterChartRuntime(
}

const locale = getters.getLocale();
const truncateLabels = axisType === "category";
const dataSetFormat = getChartDatasetFormat(getters, chart.dataSets);
const localeFormat = { format: dataSetFormat, locale };
const config = getLineOrScatterConfiguration(chart, labels, localeFormat);
const options = { format: dataSetFormat, locale, truncateLabels };
const config = getLineOrScatterConfiguration(chart, labels, options);
const labelFormat = getChartLabelFormat(getters, chart.labelRange)!;
if (axisType === "time") {
const axis = {
Expand Down
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 @@ -103,7 +103,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 @@ -151,7 +151,7 @@ export function getDefaultChartJsRuntime(
type: chart.type as ChartType,
options,
data: {
labels: labels.map(truncateLabel),
labels: truncateLabels ? labels.map(truncateLabel) : labels,
datasets: [],
},
platform: undefined as unknown as typeof BasePlatform, // This key is optional and will be set by chart.js
Expand Down
32 changes: 30 additions & 2 deletions tests/figures/chart/chart_plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,14 @@ 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 { zoneToXc } from "../../../src/helpers";
import { FIGURE_ID_SPLITTER, MAX_CHAR_LABEL } from "../../../src/constants";
import { toZone, zoneToXc } from "../../../src/helpers";
import { BarChart } from "../../../src/helpers/figures/charts";
import { ChartPlugin } from "../../../src/plugins/core";
import { ComboChartRuntime } from "../../../src/types/chart/combo_chart";
import { ScatterChartRuntime } from "../../../src/types/chart/scatter_chart";
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 @@ -2020,6 +2021,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 64d67ce

Please sign in to comment.