From cca79df18b9a5cee61baee697b2f23daf3173f77 Mon Sep 17 00:00:00 2001 From: 81reap Date: Wed, 12 Aug 2026 21:39:57 -0400 Subject: [PATCH] fix(chart) :: line series up on a category axis for every chart type --- CHANGELOG.md | 1 + sqlpage/apexcharts.js | 46 +++-- tests/end-to-end/chart-component.spec.ts | 114 ++++++++++++- tests/js/chart_series.spec.ts | 209 ++++++++++++++++++----- 4 files changed, 310 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97f3b443..b7a5e5ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Form `options_source` URLs now preserve existing query parameters when adding the dynamic `search` parameter. - Map coordinates that are not a pair of numbers, like a latitude with no longitude, are now reported in the browser console and skipped, instead of breaking the whole map. - Stacked charts now stack their series by `x` value instead of by point order, which used to give wrong totals when a series was missing a point. + - `line`, `area`, `scatter`, `bubble` and `heatmap` charts with text labels on the x axis now line their series up by label, leaving a gap where a series skips one. - `column` charts now display vertical bars instead of nothing at all. - `stacked` is now ignored on chart types that cannot stack, instead of displaying an empty chart. - Screen readers now announce the title of the modal component instead of an unnamed dialog. diff --git a/sqlpage/apexcharts.js b/sqlpage/apexcharts.js index f4f0cc61..cd1d1d58 100644 --- a/sqlpage/apexcharts.js +++ b/sqlpage/apexcharts.js @@ -38,6 +38,14 @@ sqlpage_chart = (() => { const STACKABLE_CHART_TYPES = ["line", "area", "bar"]; const APEXCHARTS_TYPE_ALIASES = { column: "bar" }; + const Y_WHEN_A_SERIES_SKIPS_A_LABEL = { + bar: 0, + line: null, + area: null, + scatter: null, + bubble: null, + heatmap: null, + }; /** @typedef {number|string|Date} XValue */ /** @typedef { {name:string, data:{x:XValue,y:number|null,z?:number}[]} } ChartSeries */ @@ -46,6 +54,9 @@ sqlpage_chart = (() => { /** @param {XValue} x @returns {number|string} equal x values share a key */ const x_key = (x) => (x instanceof Date ? x.getTime() : x); + /** @param {ChartSeries[]} series */ + const x_is_text = (series) => typeof series[0]?.data[0]?.x === "string"; + /** * @param {ChartSeries[]} series * @returns {XValue[]} every x the series hold, in their own order where they @@ -66,13 +77,15 @@ sqlpage_chart = (() => { /** * ApexCharts pairs points across series by index rather than by x, so a - * series that skips an x stacks onto the wrong one. Give every series the - * same x values, counting an x it never measured as zero. + * series that skips an x lands on the wrong one. Give every series the same + * amount of x values. * * @param {ChartSeries[]} series + * @param {number|null} y_when_missing what a series with no value at an x is + * worth there: zero to add nothing to a stack, null to leave a gap. * @returns {ChartSeries[]} */ - function align_series(series) { + function align_series(series, y_when_missing) { const all_x = merged_x_values(series); return series.map(({ name, data }) => { const by_x = new Map(data.map((point) => [x_key(point.x), point])); @@ -80,15 +93,28 @@ sqlpage_chart = (() => { name, data: all_x.map((x) => { const point = by_x.get(x_key(x)); - return { ...point, x, y: point?.y || 0 }; + return { ...point, x, y: point?.y ?? y_when_missing }; }), }; }); } + /** + * @param {ChartSeries[]} series + * @param {string} chart_type + * @param {boolean} is_stacked + * @returns {ChartSeries[]} + */ + function align_series_for(series, chart_type, is_stacked) { + if (is_stacked) return align_series(series, 0); + if (x_is_text(series) && chart_type in Y_WHEN_A_SERIES_SKIPS_A_LABEL) + return align_series(series, Y_WHEN_A_SERIES_SKIPS_A_LABEL[chart_type]); + return series; + } + // The unit tests load this file as a CommonJS module; browsers have no `module`. if (typeof module !== "undefined") - module.exports = { align_series, merged_x_values }; + module.exports = { align_series, align_series_for, merged_x_values }; /** @param {HTMLElement} c */ function build_sqlpage_chart(c) { @@ -129,16 +155,12 @@ sqlpage_chart = (() => { let series = Object.values(series_map); let labels; - const categories = - series.length > 0 && typeof series[0].data[0].x === "string"; + const categories = x_is_text(series); if (chart_type === "pie") { labels = data.points.map(([name, x, _y]) => x || name); series = data.points.map(([_name, _x, y]) => Number.parseFloat(y)); - } else if ( - series.length > 1 && - (is_stacked || (categories && chart_type === "bar")) - ) - series = align_series(series); + } else if (series.length > 1) + series = align_series_for(series, chart_type, is_stacked); const options = { chart: { diff --git a/tests/end-to-end/chart-component.spec.ts b/tests/end-to-end/chart-component.spec.ts index 8792a230..3c32f101 100644 --- a/tests/end-to-end/chart-component.spec.ts +++ b/tests/end-to-end/chart-component.spec.ts @@ -106,17 +106,21 @@ async function renderChart( p.y, ]), })); - const drawnPerSeries = series.map(({ name }) => ({ - name, - heights: [ + const drawnPerSeries = series.map(({ name }) => { + const markers = [ ...container.querySelectorAll( - `.apexcharts-series[seriesName='${name}'] .apexcharts-marker`, + `.apexcharts-series[seriesName='${name}'] .apexcharts-series-markers > .apexcharts-marker`, ), - ].map((m) => Math.round(m.getBBox().y)), - })); + ].map((m) => m.getBBox()); + return { + name, + lefts: markers.map((b) => Math.round(b.x)), + heights: markers.map((b) => Math.round(b.y)), + }; + }); const shapes = [ ...container.querySelectorAll( - ".apexcharts-bar-area, .apexcharts-rangebar-area", + ".apexcharts-bar-area, .apexcharts-rangebar-area, .apexcharts-treemap-rect", ), ].map((shape) => { const { x, y, width, height } = shape.getBBox(); @@ -252,6 +256,102 @@ test("stacks a bar series on the categories it skipped", async ({ page }) => { ]); }); +test("lines an unstacked series up with the categories it skipped", async ({ + page, +}) => { + const chart = await renderChart(page, { type: "line" }, [ + ...A_IN_EVERY_QUARTER, + ...B_MISSING_THE_FIRST_QUARTER, + ]); + + expect(chart.failures).toEqual([]); + expect(chart.series[1].points).toEqual([ + ["Q1", null], + ["Q2", 20], + ["Q3", 30], + ]); +}); + +test("draws nothing where an unstacked series has no value", async ({ + page, +}) => { + const chart = await renderChart(page, { type: "line" }, [ + ...A_IN_EVERY_QUARTER, + ...B_MISSING_THE_FIRST_QUARTER, + ]); + const [a, b] = chart.drawnPerSeries; + + expect(a.lefts).toHaveLength(3); + expect(b.lefts).toEqual(a.lefts.slice(1)); +}); + +test("keeps a measured zero apart from a missing value", async ({ page }) => { + const chart = await renderChart(page, { type: "line" }, [ + ...A_IN_EVERY_QUARTER, + ["B", "Q2", 0], + ["B", "Q3", 30], + ]); + const [a, b] = chart.drawnPerSeries; + + expect(chart.series[1].points).toEqual([ + ["Q1", null], + ["Q2", 0], + ["Q3", 30], + ]); + expect(b.lefts).toEqual(a.lefts.slice(1)); +}); + +for (const type of ["area", "scatter", "heatmap"]) { + test(`lines up the series of a ${type} chart on a category axis`, async ({ + page, + }) => { + const chart = await renderChart(page, { type }, [ + ...A_IN_EVERY_QUARTER, + ...B_MISSING_THE_FIRST_QUARTER, + ]); + + expect(chart.failures).toEqual([]); + expect(chart.series[1].points.map((p) => p[0])).toEqual(["Q1", "Q2", "Q3"]); + }); +} + +test("keeps the bubble size of the points it lined up", async ({ page }) => { + const chart = await renderChart(page, { type: "bubble" }, [ + ["A", "Q1", 1, 30], + ["A", "Q2", 2, 30], + ["B", "Q2", 5, 70], + ]); + + expect(chart.failures).toEqual([]); + expect(chart.series[1].points).toEqual([ + ["Q1", null], + ["Q2", 5], + ]); +}); + +test("leaves a rangeBar chart on a category axis alone", async ({ page }) => { + const chart = await renderChart( + page, + { type: "rangeBar", time: true }, + TASKS_OVER_TIME, + ); + + expect(chart.failures).toEqual([]); + expect(chart.shapes).toHaveLength(2); +}); + +test("leaves a treemap chart alone", async ({ page }) => { + const chart = await renderChart(page, { type: "treemap" }, [ + ["North America", "United States", 35], + ["North America", "Canada", 15], + ["Europe", "France", 30], + ["Europe", "Germany", 55], + ]); + + expect(chart.failures).toEqual([]); + expect(chart.shapes).toHaveLength(4); +}); + test("draws a rangeBar chart that asks to be stacked", async ({ page }) => { const chart = await renderChart( page, diff --git a/tests/js/chart_series.spec.ts b/tests/js/chart_series.spec.ts index 9f91d34a..7ef7ddc1 100644 --- a/tests/js/chart_series.spec.ts +++ b/tests/js/chart_series.spec.ts @@ -11,11 +11,17 @@ Object.assign(globalThis, browser_globals_apexcharts_reads_when_it_loads); const require = createRequire(import.meta.url); const { align_series, + align_series_for, merged_x_values, } = require("../../sqlpage/apexcharts.js"); +const ADDS_NOTHING_TO_THE_STACK = 0; +const LEAVES_A_GAP = null; +const STACKED = true; +const UNSTACKED = false; + type XValue = number | string | Date; -type Point = { x: XValue; y: number | string | null; z?: number }; +type Point = { x: XValue; y: number | string | null | number[]; z?: number }; type Series = { name: string; data: Point[] }; const series = (name: string, ...data: Point[]): Series => ({ name, data }); @@ -67,20 +73,26 @@ test("merged_x_values ignores series that hold no points", () => { }); test("align_series gives every series a point at every x (#727)", () => { - const [a, b] = align_series([ - series("a", { x: "Q1", y: 1 }, { x: "Q2", y: 2 }), - series("b", { x: "Q2", y: 3 }), - ]); + const [a, b] = align_series( + [ + series("a", { x: "Q1", y: 1 }, { x: "Q2", y: 2 }), + series("b", { x: "Q2", y: 3 }), + ], + ADDS_NOTHING_TO_THE_STACK, + ); assert.deepEqual(xs(a), ["Q1", "Q2"]); assert.deepEqual(xs(b), ["Q1", "Q2"]); }); -test("align_series counts an x a series skipped as zero (#727)", () => { - const [, b] = align_series([ - series("a", { x: "Q1", y: 1 }, { x: "Q2", y: 2 }), - series("b", { x: "Q2", y: 3 }), - ]); +test("align_series counts an x a stacked series skipped as zero (#727)", () => { + const [, b] = align_series( + [ + series("a", { x: "Q1", y: 1 }, { x: "Q2", y: 2 }), + series("b", { x: "Q2", y: 3 }), + ], + ADDS_NOTHING_TO_THE_STACK, + ); assert.deepEqual(b.data, [ { x: "Q1", y: 0 }, @@ -88,47 +100,80 @@ test("align_series counts an x a series skipped as zero (#727)", () => { ]); }); -test("align_series counts a null value as a value the series never measured", () => { - const [, b] = align_series([ - series("a", { x: "Q1", y: 1 }), - series("b", { x: "Q1", y: null }), - ]); +test("align_series leaves a gap where an unstacked series has no value", () => { + const [, b] = align_series( + [ + series("a", { x: "Q1", y: 1 }, { x: "Q2", y: 2 }), + series("b", { x: "Q2", y: 3 }), + ], + LEAVES_A_GAP, + ); - assert.deepEqual(b.data, [{ x: "Q1", y: 0 }]); + assert.deepEqual(b.data, [ + { x: "Q1", y: null }, + { x: "Q2", y: 3 }, + ]); }); -test("align_series counts a blank value as zero", () => { - const [, b] = align_series([ - series("a", { x: "Q1", y: 1 }), - series("b", { x: "Q1", y: "" }), +test("align_series keeps a measured zero apart from a missing value", () => { + const [, b] = align_series( + [ + series("a", { x: "Q1", y: 1 }, { x: "Q2", y: 2 }), + series("b", { x: "Q2", y: 0 }), + ], + LEAVES_A_GAP, + ); + + assert.deepEqual(b.data, [ + { x: "Q1", y: null }, + { x: "Q2", y: 0 }, ]); +}); + +test("align_series counts a null value as missing", () => { + const [, b] = align_series( + [series("a", { x: "Q1", y: 1 }), series("b", { x: "Q1", y: null })], + ADDS_NOTHING_TO_THE_STACK, + ); assert.deepEqual(b.data, [{ x: "Q1", y: 0 }]); }); +test("align_series keeps a blank value the series wrote", () => { + const [, b] = align_series( + [series("a", { x: "Q1", y: 1 }), series("b", { x: "Q1", y: "" })], + ADDS_NOTHING_TO_THE_STACK, + ); + + assert.deepEqual(b.data, [{ x: "Q1", y: "" }]); +}); + test("align_series keeps a value the series wrote as text", () => { - const [, b] = align_series([ - series("a", { x: "Q1", y: 1 }), - series("b", { x: "Q1", y: "7" }), - ]); + const [, b] = align_series( + [series("a", { x: "Q1", y: 1 }), series("b", { x: "Q1", y: "7" })], + ADDS_NOTHING_TO_THE_STACK, + ); assert.deepEqual(b.data, [{ x: "Q1", y: "7" }]); }); test("align_series keeps the third dimension of points it did not fill in", () => { - const [a] = align_series([ - series("a", { x: "Q1", y: 1, z: 42 }), - series("b", { x: "Q2", y: 2 }), - ]); + const [a] = align_series( + [series("a", { x: "Q1", y: 1, z: 42 }), series("b", { x: "Q2", y: 2 })], + LEAVES_A_GAP, + ); assert.equal(a.data[0].z, 42); }); test("align_series matches dates by value rather than by identity", () => { - const [a, b] = align_series([ - series("a", { x: new Date("2024-03-01"), y: 1 }), - series("b", { x: new Date("2024-03-01"), y: 2 }), - ]); + const [a, b] = align_series( + [ + series("a", { x: new Date("2024-03-01"), y: 1 }), + series("b", { x: new Date("2024-03-01"), y: 2 }), + ], + ADDS_NOTHING_TO_THE_STACK, + ); assert.equal(a.data.length, 1); assert.equal(b.data.length, 1); @@ -136,9 +181,10 @@ test("align_series matches dates by value rather than by identity", () => { }); test("align_series leaves a lone series in the order it arrived (#930)", () => { - const [only] = align_series([ - series("a", { x: "Q2", y: 1 }, { x: "Q1", y: 2 }), - ]); + const [only] = align_series( + [series("a", { x: "Q2", y: 1 }, { x: "Q1", y: 2 })], + LEAVES_A_GAP, + ); assert.deepEqual(only.data, [ { x: "Q2", y: 1 }, @@ -152,7 +198,7 @@ test("align_series returns series that already share every x unchanged", () => { series("b", { x: "Q3", y: 4 }, { x: "Q1", y: 5 }, { x: "Q2", y: 6 }), ]; - assert.deepEqual(align_series(given), given); + assert.deepEqual(align_series(given, ADDS_NOTHING_TO_THE_STACK), given); }); test("align_series does not mutate the series it is given", () => { @@ -162,19 +208,100 @@ test("align_series does not mutate the series it is given", () => { ]; const before = JSON.stringify(given); - align_series(given); + align_series(given, LEAVES_A_GAP); assert.equal(JSON.stringify(given), before); }); test("align_series keeps the last of duplicated x values", () => { - const [a] = align_series([ - series("a", { x: "Q1", y: 1 }, { x: "Q1", y: 9 }), - series("b", { x: "Q2", y: 2 }), - ]); + const [a] = align_series( + [ + series("a", { x: "Q1", y: 1 }, { x: "Q1", y: 9 }), + series("b", { x: "Q2", y: 2 }), + ], + ADDS_NOTHING_TO_THE_STACK, + ); assert.deepEqual(a.data, [ { x: "Q1", y: 9 }, { x: "Q2", y: 0 }, ]); }); + +test("align_series_for gives a stacked series a zero at every x it skipped", () => { + const [, b] = align_series_for( + [series("a", { x: 1, y: 1 }, { x: 2, y: 2 }), series("b", { x: 2, y: 3 })], + "area", + STACKED, + ); + + assert.deepEqual(b.data, [ + { x: 1, y: 0 }, + { x: 2, y: 3 }, + ]); +}); + +for (const type of ["line", "area", "scatter", "bubble", "heatmap"]) { + test(`align_series_for leaves a gap where a ${type} series skips a label`, () => { + const [, b] = align_series_for( + [ + series("a", { x: "Q1", y: 1 }, { x: "Q2", y: 2 }), + series("b", { x: "Q2", y: 3 }), + ], + type, + UNSTACKED, + ); + + assert.deepEqual(b.data, [ + { x: "Q1", y: null }, + { x: "Q2", y: 3 }, + ]); + }); +} + +test("align_series_for counts a label a bar series skipped as zero", () => { + const [, b] = align_series_for( + [ + series("a", { x: "Q1", y: 1 }, { x: "Q2", y: 2 }), + series("b", { x: "Q2", y: 3 }), + ], + "bar", + UNSTACKED, + ); + + assert.deepEqual(b.data, [ + { x: "Q1", y: 0 }, + { x: "Q2", y: 3 }, + ]); +}); + +test("align_series_for leaves a treemap's regions their own labels", () => { + const regions = [ + series( + "North America", + { x: "United States", y: 35 }, + { x: "Canada", y: 15 }, + ), + series("Europe", { x: "France", y: 30 }, { x: "Germany", y: 55 }), + ]; + + assert.equal(align_series_for(regions, "treemap", UNSTACKED), regions); +}); + +test("align_series_for leaves a rangeBar timeline alone", () => { + const tasks = [ + series("Design", { x: "Alice", y: [1, 5] }), + series("Build", { x: "Bob", y: [4, 9] }), + ]; + + assert.equal(align_series_for(tasks, "rangeBar", UNSTACKED), tasks); +}); + +test("align_series_for leaves unstacked series without labels alone", () => { + const lines = [ + series("a", { x: 1, y: 1 }, { x: 2, y: 2 }), + series("b", { x: 2, y: 3 }), + ]; + + assert.equal(align_series_for(lines, "line", UNSTACKED), lines); +});