-
Notifications
You must be signed in to change notification settings - Fork 67
[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?
Conversation
59e8688 to
5e593d6
Compare
Task Description This commit aims to add the possibility for the user to customize the axes scale (linear vs logarithmic type and mix/max values) for linear scale. We also add the possibility to show/hide major and minor gridlines (minor gridlines being only available for linear scale and line/scatter chart (for x axis), while avalaible for bar/line/scatter/combo chart for both y axes. Related Task Task: 5159370
This commit extends the axis customization to date type x scale Task: 5159370
5e593d6 to
b807af0
Compare
hokolomopo
left a comment
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.
👋
Scroll scroll
Cool feature! Not what the spec was tho, so let's talk with François sometime 🙂
| return undefined; | ||
| } | ||
| if (this.isTimeAxis) { | ||
| const timestamp = this.getTimestampFromInput(input); |
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.
You should really convert that to a spreadsheet date number (1 is 1/1/0900) - not a js timestamp. If I add a limit then remove the date format, I get an huge value that doesn't make sense (eg. 1756425600000). If you use spreadsheet date numbers, it would make sense even after removing the format.
src/components/side_panel/chart/building_blocks/axis_design/axis_design_editor.xml
Show resolved
Hide resolved
| import { LineChartDefinition, LineChartRuntime } from "./line_chart"; | ||
|
|
||
| export interface ScatterChartDefinition | ||
| extends Omit<LineChartDefinition, "type" | "stacked" | "cumulative"> { |
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.
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 ?
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.
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 ...
We can discuss this once again but it's not something I decided "dans mon coin" :/
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.
That's fine then, but do it in a separate task :) And maybe in 19.0 where the zoom was introduced ?
| readonly min?: number; | ||
| readonly max?: number; | ||
| readonly scaleType?: AxisScaleType; | ||
| readonly grid?: AxisGridDesign; |
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/minorGridLine rather than design.grid.major
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.
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 :)
| @@ -0,0 +1,69 @@ | |||
| import { Chart } from "chart.js"; | |||
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.
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
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.
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.
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.
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

Task Description
This commit aims to add the possibility for the user to customize the axes scale (linear vs logarithmic type and mix/max values) for linear scale. We also add the possibility to show/hide major and minor gridlines (minor gridlines being only available for linear scale and line/scatter chart (for x axis), while available for bar/line/scatter/combo chart for both y axes.
Related Task