-
-
Notifications
You must be signed in to change notification settings - Fork 175
fix(chart): make xticks count tick positions #1440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 ( | ||
|
|
@@ -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; | ||
| } | ||
|
|
||
| /** | ||
| * @param {ChartSeries[]} series | ||
| * @returns {XValue[]} every x the series hold, in their own order where they | ||
|
|
@@ -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") { | ||
|
|
@@ -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); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the docs say this is tick interval not the number of ticks
|
||
| const chart = new ApexCharts(chartContainer, options); | ||
| chart.render(); | ||
| if (window.charts) window.charts.push(chart); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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", | ||
| ); | ||
| }); | ||
|
|
@@ -55,8 +70,22 @@ test("does not turn category-oriented charts into numeric axes", () => { | |
| const numeric = [series("a", { x: 1, y: 1 })]; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding a test for |
||
|
|
||
| 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", () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numericis only possible ifis_horizontalisfalse(see line 70 before or 79 after)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may also not need the
- 1https://github.com/apexcharts/apexcharts.js/blob/cff2224488c1330bfeb2d72e30c1878e0a091847/src/modules/axes/AxesUtils.js#L124
There was a problem hiding this comment.
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