-
Notifications
You must be signed in to change notification settings - Fork 44
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
[IMP] charts: limit trending line degree range #5022
base: master
Are you sure you want to change the base?
Conversation
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.
tchou tchou 🚂
@@ -2,6 +2,7 @@ import { Component, useState } from "@odoo/owl"; | |||
import { getColorsPalette, getNthColor, setColorAlpha, toHex } from "../../../../helpers"; |
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: in commit message
fixed to the minimum value between 10 and 1 below the number of
src/components/side_panel/chart/chart_with_axis/design_panel.ts
Outdated
Show resolved
Hide resolved
const maxOrder = this.getMaxPolynomialDegree(); | ||
if (order > maxOrder) { | ||
element.value = `${this.getTrendLineConfiguration()?.order ?? maxOrder}`; | ||
return; |
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.
Wait so we change the value of the input, but do not call updateTrendLineValue ? Won't we have a mismatch between the chart and the input ? (same for if (order < 2)
)
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.
No, as it doesn't update the chart, and it put back the original value (if existing, which is almost always the case)
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.
Oh yeah we try to use this.getTrendLineConfiguration()?.order
(which I'm gressing is from the chart definition).
Still, shouldn't we set the max/min value instead of going back to the old value ? If I write degree 40 in my input i'd expect the polynomial go go to the max degree rather that goign back to 2. Or maybe it's just me ?
Task Description This task aims to add a maximum value for the trending line degree (for a polynomial model), fixed to the minimum value between 10 and 1 below the number of points in the series. Related Task Task: 4207820
e7e7675
to
dd2616f
Compare
Task Description
This task aims to add a maximum value for the trending line degree (for a polynomial model), fixed to the minimum value of 10 and 1 below of the number of point in the series.
Related Task
review checklist