-
Notifications
You must be signed in to change notification settings - Fork 68
[IMP] charts: add scales range, type and grid display settings #7350
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
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 |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { LineChartDefinition, LineChartRuntime } from "./line_chart"; | ||
|
|
||
| export interface ScatterChartDefinition | ||
| extends Omit<LineChartDefinition, "type" | "stacked" | "cumulative"> { | ||
|
Contributor
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. Uh ? scatter cahrts are not zoomable ? I did a fix and all for zoomable scatter charts https://www.odoo.com/odoo/2328/tasks/5143146 Why change that? And why in this task ?
Contributor
Author
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. This was discussed with FRGI. We decided to remove the zoom of the scatter chart because in a scatter it's strange to be able to zoom on the x axis but not on the y-axis, and therefore this task to set the min and max value of the scatter chart axes make the zoom slicer useless ...
Contributor
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. That's fine then, but do it in a separate task :) And maybe in 19.0 where the zoom was introduced ? |
||
| extends Omit<LineChartDefinition, "type" | "stacked" | "cumulative" | "zoomable"> { | ||
| readonly type: "scatter"; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| import { Chart } from "chart.js"; | ||
|
Contributor
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. Are minor grid line really not a default chartjs option ? Idk if it's worth doing a plugin for that, IMO minor grid lines are kinda useless
Contributor
Author
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. Minor gridline is really usefull in a scatter chart and is a features GSheet and Excell both have ... Anyway, we can make it in another way with more ticks and some tikslabel being undefined/empty if you dont want the plugin.
Contributor
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. If Francois thinks it's interesting let's do it 🤷 Side note (not sure if I already mentioned it) but the color you chose for minor grid lines is barely visible on my low-luminosity/contrast screen |
||
| import { Color } from "../../../.."; | ||
|
|
||
| interface MinorGridOptions { | ||
| display?: boolean; | ||
| color?: Color; | ||
| } | ||
|
|
||
| export const chartMinorGridPlugin = { | ||
| id: "o-spreadsheet-minor-gridlines", | ||
| beforeDatasetsDraw(chart: Chart) { | ||
| const ctx = chart.ctx; | ||
| const chartArea = chart.chartArea; | ||
| if (!chartArea) { | ||
| return; | ||
| } | ||
|
|
||
| for (const scaleId in chart.scales) { | ||
| const scale = chart.scales[scaleId]; | ||
| const options: any = scale.options; | ||
| const minor: MinorGridOptions | undefined = options?.grid?.minor; | ||
| if (!minor?.display) { | ||
| continue; | ||
| } | ||
| const ticks = scale.ticks; | ||
| if (!ticks || ticks.length < 2) { | ||
| continue; | ||
| } | ||
|
|
||
| ctx.save(); | ||
| ctx.lineWidth = 1 / 2; | ||
| ctx.strokeStyle = minor.color ?? options?.grid?.color ?? "#bdbdbd"; | ||
| ctx.setLineDash([]); | ||
| ctx.globalAlpha = 0.6; | ||
|
|
||
| const drawVerticalLine = (x: number) => { | ||
| ctx.beginPath(); | ||
| ctx.moveTo(x, chartArea.top); | ||
| ctx.lineTo(x, chartArea.bottom); | ||
| ctx.stroke(); | ||
| }; | ||
| const drawHorizontalLine = (y: number) => { | ||
| ctx.beginPath(); | ||
| ctx.moveTo(chartArea.left, y); | ||
| ctx.lineTo(chartArea.right, y); | ||
| ctx.stroke(); | ||
| }; | ||
|
|
||
| for (let i = 0; i < ticks.length - 1; i++) { | ||
| const start = scale.getPixelForTick(i); | ||
| const end = scale.getPixelForTick(i + 1); | ||
| if (!isFinite(start) || !isFinite(end)) { | ||
| continue; | ||
| } | ||
| for (let j = 1; j < 4; j++) { | ||
| const ratio = j / 4; | ||
| const position = start + (end - start) * ratio; | ||
| if (scale.isHorizontal()) { | ||
| drawVerticalLine(position); | ||
| } else { | ||
| drawHorizontalLine(position); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ctx.restore(); | ||
| } | ||
| }, | ||
| }; | ||
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.
nitpick: probably more clear to have a boolean
showMajor/minorGridLinerather thandesign.grid.majorThere 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.
Then what about a showGrid that can be "major", "minor", "none" or "both" ?
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'd put something more explicit for both like
minorAndMajor, but yeah that sounds good :)