-
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?
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 |
|---|---|---|
| @@ -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(); | ||
| } | ||
| }, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,20 @@ | ||
| import { _t } from "@odoo/o-spreadsheet-engine"; | ||
| import { CHART_AXIS_TITLE_FONT_SIZE } from "@odoo/o-spreadsheet-engine/constants"; | ||
| import { AxisType, LineChartRuntime } from "@odoo/o-spreadsheet-engine/types/chart"; | ||
| import { SpreadsheetChildEnv } from "@odoo/o-spreadsheet-engine/types/spreadsheet_env"; | ||
| import { Component, useState } from "@odoo/owl"; | ||
| import { deepCopy } from "../../../../../helpers"; | ||
| import { ChartWithAxisDefinition, DispatchResult, TitleDesign, UID } from "../../../../../types"; | ||
| import { getDefinedAxis } from "../../../../../helpers/figures/charts"; | ||
| import { | ||
| AxisDesign, | ||
| AxisScaleType, | ||
| ChartWithAxisDefinition, | ||
| DispatchResult, | ||
| TitleDesign, | ||
| UID, | ||
| } from "../../../../../types"; | ||
| import { BadgeSelection } from "../../../components/badge_selection/badge_selection"; | ||
| import { Checkbox } from "../../../components/checkbox/checkbox"; | ||
| import { Section } from "../../../components/section/section"; | ||
| import { ChartTitle } from "../chart_title/chart_title"; | ||
|
|
||
|
|
@@ -21,7 +32,7 @@ interface Props { | |
|
|
||
| export class AxisDesignEditor extends Component<Props, SpreadsheetChildEnv> { | ||
| static template = "o-spreadsheet-AxisDesignEditor"; | ||
| static components = { Section, ChartTitle, BadgeSelection }; | ||
| static components = { Section, ChartTitle, BadgeSelection, Checkbox }; | ||
| static props = { chartId: String, definition: Object, updateChart: Function, axesList: Array }; | ||
|
|
||
| state = useState({ currentAxis: "x" }); | ||
|
|
@@ -42,7 +53,7 @@ export class AxisDesignEditor extends Component<Props, SpreadsheetChildEnv> { | |
|
|
||
| getAxisTitle() { | ||
| const axesDesign = this.props.definition.axesDesign ?? {}; | ||
| return axesDesign[this.state.currentAxis]?.title.text || ""; | ||
| return axesDesign[this.state.currentAxis]?.title?.text || ""; | ||
| } | ||
|
|
||
| updateAxisTitle(text: string) { | ||
|
|
@@ -65,4 +76,193 @@ export class AxisDesignEditor extends Component<Props, SpreadsheetChildEnv> { | |
| }; | ||
| this.props.updateChart(this.props.chartId, { axesDesign }); | ||
| } | ||
|
|
||
| get axisMin(): string | number | undefined { | ||
| const min = this.currentAxisDesign?.min; | ||
| return this.isTimeAxis ? this.formatAxisBoundary(min) : min; | ||
| } | ||
|
|
||
| get axisMax(): string | number | undefined { | ||
| const max = this.currentAxisDesign?.max; | ||
| return this.isTimeAxis ? this.formatAxisBoundary(max) : max; | ||
| } | ||
|
|
||
| get axisScaleType(): AxisScaleType { | ||
| return this.currentAxisDesign?.scaleType ?? "linear"; | ||
| } | ||
|
|
||
| get axisBoundsInputType(): "number" | "date" { | ||
| return this.isTimeAxis ? "date" : "number"; | ||
| } | ||
|
|
||
| get axisBoundsInputStep(): string | null { | ||
| return this.isTimeAxis ? "1" : null; | ||
| } | ||
|
|
||
| get isMajorGridEnabled(): boolean { | ||
| const designValue = this.currentAxisDesign?.grid?.major; | ||
| if (designValue !== undefined) { | ||
| return designValue; | ||
| } | ||
| return this.getDefaultMajorGridValue(this.state.currentAxis); | ||
| } | ||
|
|
||
| get isMinorGridEnabled(): boolean { | ||
| return !!this.currentAxisDesign?.grid?.minor; | ||
| } | ||
|
|
||
| get majorGridLabel() { | ||
| return this.state.currentAxis === "x" | ||
| ? _t("Vertical major gridlines") | ||
| : _t("Horizontal major gridlines"); | ||
| } | ||
|
|
||
| get minorGridLabel() { | ||
| return this.state.currentAxis === "x" | ||
| ? _t("Vertical minor gridlines") | ||
| : _t("Horizontal minor gridlines"); | ||
| } | ||
|
|
||
| updateAxisMin(ev: InputEvent) { | ||
| const parsed = this.parseAxisBoundaryValue(ev); | ||
| if (parsed === null) { | ||
| return; | ||
| } | ||
| const axesDesign = deepCopy(this.props.definition.axesDesign) ?? {}; | ||
| axesDesign[this.state.currentAxis] = { | ||
| ...axesDesign[this.state.currentAxis], | ||
| min: parsed, | ||
| }; | ||
| this.props.updateChart(this.props.chartId, { axesDesign }); | ||
| } | ||
|
|
||
| updateAxisMax(ev: InputEvent) { | ||
| const parsed = this.parseAxisBoundaryValue(ev); | ||
| if (parsed === null) { | ||
| return; | ||
| } | ||
| const axesDesign = deepCopy(this.props.definition.axesDesign) ?? {}; | ||
| axesDesign[this.state.currentAxis] = { | ||
| ...axesDesign[this.state.currentAxis], | ||
| max: parsed, | ||
| }; | ||
| this.props.updateChart(this.props.chartId, { axesDesign }); | ||
| } | ||
|
|
||
| updateAxisScaleType(ev: InputEvent) { | ||
| const type = (ev.target as HTMLSelectElement).value as AxisScaleType; | ||
| const axesDesign = deepCopy(this.props.definition.axesDesign) ?? {}; | ||
| axesDesign[this.state.currentAxis] = { | ||
| ...axesDesign[this.state.currentAxis], | ||
| scaleType: type === "linear" ? undefined : type, | ||
| }; | ||
| this.props.updateChart(this.props.chartId, { axesDesign }); | ||
| } | ||
|
|
||
| toggleMajorGrid(major: boolean) { | ||
| const axesDesign = deepCopy(this.props.definition.axesDesign) ?? {}; | ||
| axesDesign[this.state.currentAxis] = { | ||
| ...axesDesign[this.state.currentAxis], | ||
| grid: { | ||
| ...axesDesign[this.state.currentAxis]?.grid, | ||
| major, | ||
| }, | ||
| }; | ||
| this.props.updateChart(this.props.chartId, { axesDesign }); | ||
| } | ||
|
|
||
| toggleMinorGrid(minor: boolean) { | ||
| const axesDesign = deepCopy(this.props.definition.axesDesign) ?? {}; | ||
| axesDesign[this.state.currentAxis] = { | ||
| ...axesDesign[this.state.currentAxis], | ||
| grid: { | ||
| ...axesDesign[this.state.currentAxis]?.grid, | ||
| minor, | ||
| }, | ||
| }; | ||
| this.props.updateChart(this.props.chartId, { axesDesign }); | ||
| } | ||
|
|
||
| private get currentAxisDesign(): AxisDesign | undefined { | ||
| return this.props.definition.axesDesign?.[this.state.currentAxis]; | ||
| } | ||
|
|
||
| private getDefaultMajorGridValue(axisId: string): boolean { | ||
| const { useLeftAxis, useRightAxis } = getDefinedAxis(this.props.definition); | ||
| if (axisId === "x") { | ||
| if ("horizontal" in this.props.definition && this.props.definition.horizontal) { | ||
| return true; | ||
| } | ||
| if (this.props.definition.type === "scatter") { | ||
| return true; | ||
| } | ||
| } else if (axisId === "y") { | ||
| return true; | ||
| } else if (axisId === "y1") { | ||
| return !useLeftAxis && useRightAxis; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| get isCategoricalAxis(): boolean { | ||
| if (this.state.currentAxis !== "x") { | ||
| return false; | ||
| } | ||
| const axisType = this.getXAxisType(); | ||
| return axisType === undefined || axisType === "category"; | ||
| } | ||
|
|
||
| get isTimeAxis(): boolean { | ||
| return this.state.currentAxis === "x" && this.getXAxisType() === "time"; | ||
| } | ||
|
|
||
| get canChangeMinorGridVisibility(): boolean { | ||
| if (this.state.currentAxis !== "x") { | ||
| return true; | ||
| } | ||
| if (this.isCategoricalAxis) { | ||
| return false; | ||
| } | ||
| const type = this.props.definition.type; | ||
| return type === "line" || type === "scatter"; | ||
| } | ||
|
|
||
| private parseAxisBoundaryValue(ev: InputEvent): number | undefined | null { | ||
| const input = ev.target as HTMLInputElement; | ||
| const value = input.value.trim(); | ||
| if (value === "") { | ||
| return undefined; | ||
| } | ||
| if (this.isTimeAxis) { | ||
| const timestamp = this.getTimestampFromInput(input); | ||
|
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. You should really convert that to a spreadsheet date number ( |
||
| return Number.isNaN(timestamp) ? null : timestamp; | ||
| } | ||
| const parsed = Number(value); | ||
| return Number.isNaN(parsed) ? null : parsed; | ||
| } | ||
|
|
||
| private formatAxisBoundary(value: number | string | undefined): string | undefined { | ||
| if (value === undefined) { | ||
| return undefined; | ||
| } | ||
| const timestamp = typeof value === "number" ? value : Date.parse(value); | ||
| if (Number.isNaN(timestamp)) { | ||
| return typeof value === "string" ? value : undefined; | ||
| } | ||
| const date = new Date(timestamp); | ||
| return date.toISOString().split("T")[0]; | ||
| } | ||
|
|
||
| private getTimestampFromInput(input: HTMLInputElement): number { | ||
| const valueAsNumber = (input as any).valueAsNumber as number | undefined; | ||
| if (typeof valueAsNumber === "number" && !Number.isNaN(valueAsNumber)) { | ||
| return valueAsNumber; | ||
| } | ||
| return Date.parse(input.value); | ||
| } | ||
|
|
||
| private getXAxisType(): AxisType | undefined { | ||
| const runtime = this.env.model.getters.getChartRuntime(this.props.chartId) as LineChartRuntime; | ||
| return runtime?.chartJsConfig.options?.scales?.x?.type as AxisType | undefined; | ||
| } | ||
| } | ||
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 :)