Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

- Chart data points can now include a `link`, which is shown as a clickable link in the point's tooltip.
- Numeric x values on Cartesian charts now explicitly use a continuous numeric axis, preventing fractional tick positions from being displayed as misleading rounded integers.
- `xticks` now represents the requested number of tick positions on numeric x-axes, rather than the number of intervals between them.

## v0.46.1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ INSERT INTO parameter(component, name, description, type, top_level, optional) S
('xtitle', 'Title of the x axis, displayed below it.', 'TEXT', TRUE, TRUE),
('ytitle', 'Title of the y axis, displayed to its left.', 'TEXT', TRUE, TRUE),
('ztitle', 'Title of the z axis, displayed in tooltips.', 'TEXT', TRUE, TRUE),
('xticks', 'Number of intervals used to generate a numeric x-axis, normally producing one more tick position. On category and time axes, this is a target for label density, so the visible label count may differ.', 'INTEGER', TRUE, TRUE),
('xticks', 'Number of tick positions on a numeric x-axis. For example, 5 requests five evenly spaced positions, including the two ends. On category and time axes, it asks ApexCharts to show up to that many labels; it may show fewer to keep labels from overlapping.', 'INTEGER', TRUE, TRUE),
('yticks', 'Number of ticks on the y axis.', 'INTEGER', TRUE, TRUE),
('ystep', 'Step between ticks on the y axis.', 'REAL', TRUE, TRUE),
('marker', 'Marker size', 'REAL', TRUE, TRUE),
Expand Down
36 changes: 27 additions & 9 deletions sqlpage/apexcharts.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,17 @@ sqlpage_chart = (() => {
/** @param {ChartSeries[]} series */
const x_is_text = (series) => typeof series[0]?.data?.[0]?.x === "string";

/** @param {ChartSeries[]} series @param {string} chart_type */
function xaxis_type_for(series, chart_type, is_timeseries, is_horizontal) {
/**
* Numeric x values need an explicit axis type to retain their proportional
* spacing; otherwise ApexCharts treats them as evenly spaced categories.
*
* @param {ChartSeries[]} series
* @param {{chart_type:string, is_timeseries:boolean, is_horizontal:boolean}} options
*/
function xaxis_type_for(
series,
{ chart_type, is_timeseries, is_horizontal },
) {
if (is_timeseries) return "datetime";
if (x_is_text(series)) return "category";
if (
Expand All @@ -73,6 +82,18 @@ sqlpage_chart = (() => {
return "numeric";
}

/**
* ApexCharts expects intervals for numeric axes, while SQLPage exposes the
* more intuitive number of tick positions to users.
*
* @param {number|undefined} xticks
* @param {string|undefined} xaxis_type
*/
function xaxis_tick_amount(xticks, xaxis_type) {
if (!xticks) return;
return xaxis_type === "numeric" ? Math.max(1, xticks - 1) : xticks;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numeric is only possible if is_horizontal is false (see line 70 before or 79 after)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do need the -1, this is documented and I added a test for it https://apexcharts.com/docs/options/xaxis/

But the bigger concern is backwards compat, let's not release this in a patch release

}

/**
* @param {ChartSeries[]} series
* @returns {XValue[]} every x the series hold, in their own order where they
Expand Down Expand Up @@ -231,12 +252,11 @@ sqlpage_chart = (() => {
let colors = palette;

let series = Object.values(series_map);
const xaxis_type = xaxis_type_for(
series,
const xaxis_type = xaxis_type_for(series, {
chart_type,
is_timeseries,
!!data.horizontal,
);
is_horizontal: !!data.horizontal,
});

let labels;
if (chart_type === "pie") {
Expand Down Expand Up @@ -393,9 +413,7 @@ sqlpage_chart = (() => {
series,
};
if (labels) options.labels = labels;
// Numeric axes count intervals; category and time axes use tickAmount as a
// target for label density.
if (data.xticks) options.xaxis.tickAmount = data.xticks;
options.xaxis.tickAmount = xaxis_tick_amount(data.xticks, xaxis_type);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the docs say this is tick interval not the number of ticks

Number of Tick Intervals to show. Note: tickAmount doesn't affect datetime xaxis types. For numeric axes, use tickAmount: 'dataPoints' to match dataPoint counts.

https://apexcharts.com/docs/options/xaxis/

const chart = new ApexCharts(chartContainer, options);
chart.render();
if (window.charts) window.charts.push(chart);
Expand Down
4 changes: 2 additions & 2 deletions tests/end-to-end/fixtures/chart/numeric-axis-xticks.sql
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
SELECT
'chart' AS component,
'test-chart' AS id,
'Explicit numeric x intervals' AS title,
'Explicit numeric x tick count' AS title,
'bar' AS type,
2 AS xticks;
3 AS xticks;

SELECT 'A' AS series, 1 AS x, 1 AS y
UNION ALL SELECT 'A', 4, 4
Expand Down
3 changes: 2 additions & 1 deletion tests/end-to-end/fixtures/chart/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,11 +175,12 @@ test("keeps irregular numeric x values proportionately spaced", async ({
);
});

test("keeps an explicit x interval count", async ({ page }) => {
test("renders the requested number of numeric x ticks", async ({ page }) => {
const chart = await renderChart(page, "numeric-axis-xticks");

expect(chart.failures).toEqual([]);
expect(chart.xaxis).toEqual({ type: "numeric", tickAmount: 2 });
expect(chart.axisLabels).toHaveLength(3);
});

test("keeps text x values as categories", async ({ page }) => {
Expand Down
39 changes: 34 additions & 5 deletions tests/js/chart_series.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,31 @@ test("uses a continuous axis for numeric Cartesian x values", () => {
const numeric = [series("a", { x: 1, y: 1 }, { x: 12, y: 12 })];

for (const type of ["line", "area", "bar", "scatter", "bubble"])
assert.equal(xaxis_type_for(numeric, type, false, false), "numeric");
assert.equal(
xaxis_type_for(numeric, {
chart_type: type,
is_timeseries: false,
is_horizontal: false,
}),
"numeric",
);
});

test("keeps text and time x values on their respective axes", () => {
assert.equal(
xaxis_type_for([series("a", { x: "Q1", y: 1 })], "bar", false, false),
xaxis_type_for([series("a", { x: "Q1", y: 1 })], {
chart_type: "bar",
is_timeseries: false,
is_horizontal: false,
}),
"category",
);
assert.equal(
xaxis_type_for([series("a", { x: 1, y: 1 })], "bar", true, false),
xaxis_type_for([series("a", { x: 1, y: 1 })], {
chart_type: "bar",
is_timeseries: true,
is_horizontal: false,
}),
"datetime",
);
});
Expand All @@ -55,8 +70,22 @@ test("does not turn category-oriented charts into numeric axes", () => {
const numeric = [series("a", { x: 1, y: 1 })];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a test for [series("a", { x: "a", y: 1 })] and checking assert.equal(value_axis_labels(3, axis), 3); instead of undefined may have caught the horizontal edge case


for (const type of ["heatmap", "rangeBar", "pie", "treemap"])
assert.equal(xaxis_type_for(numeric, type, false, false), undefined);
assert.equal(xaxis_type_for(numeric, "bar", false, true), undefined);
assert.equal(
xaxis_type_for(numeric, {
chart_type: type,
is_timeseries: false,
is_horizontal: false,
}),
undefined,
);
assert.equal(
xaxis_type_for(numeric, {
chart_type: "bar",
is_timeseries: false,
is_horizontal: true,
}),
undefined,
);
});

test("merged_x_values keeps the order the series agree on", () => {
Expand Down