[REF] charts: revamp chart management #1373

pro-odoo · 2022-05-20T10:08:30Z

Description:

Since the addition of scorecard and gauge chart, it becomes a mess to
manage different types of charts. There is a duplication of many concepts,
like getters (getBasicChartDefinition, getGaugeChartDefinition, ...),
local state (charts, scorecard, gauge), ...
In addition, it was not possible to add a new type of chart from outside
o-spreadsheet (for example add Odoo charts).

This commit introduce something similar to cellFactory => a chart Factory.
Based on the type of the chart in the definition of a chart, an instance of
a Chart is created and stored in the local state of chart core plugin.
A Chart is an instance of a class (which inherits from AbstractChart),
which implements some functions that will be called by the plugin.

For each Chart class, a Chart Runtime class should be created to
compute the runtime data needed by the components.
A Side panel should also be created (a config panel and a design panel).

review checklist

robodoo · 2022-05-20T10:09:09Z

Pull request status dashboard.

pro-odoo · 2022-05-31T11:44:25Z

Some work to do in Odoo:

Update the code to add an Odoo link to a chart side panel
Migrations of commands which are update_chart

LucasLefevre

Coucou first review :)
Really nice split

src/helpers/charts/abstract_chart.ts

src/helpers/charts/chart_factory.ts

src/helpers/charts/abstract_chart.ts

src/types/chart/gauge_chart.ts

src/types/chart/chart.ts

tests/collaborative/clipboard.test.ts

tests/components/charts.test.ts

src/plugins/core/chart.ts

hokolomopo

Fast-ish review, I didn't really check the tests

demo/data.js

src/helpers/charts/abstract_chart.ts

src/components/side_panel/chart/main_chart_panel/main_chart_panel.ts

src/components/figures/chart/chartJs/chartjs.ts

src/helpers/charts/pie_chart.ts

src/index.ts

src/types/chart/common_chart.ts

rrahir · 2022-06-07T07:32:58Z

Add task id

LucasLefevre

going to miam miam 🍔

src/helpers/charts/bar_chart.ts

src/helpers/charts/chart_ui_common.ts

src/index.ts

src/helpers/charts/scorecard_chart.ts

src/helpers/charts/abstract_chart.ts

src/helpers/range.ts

src/helpers/charts/bar_chart.ts

The CSS to transform text overflow in menus to ellipsis wasn't working. The sub-menu arrow was also not displayed if the name of the menu was too long Odoo task 2877337 closes #1403 Signed-off-by: Lucas Lefèvre (lul) <[email protected]>

rrahir

My current remaining comments - lul pointed out some of them. Don't want him to negate my review completely :p

src/helpers/charts/chart_factory.ts

rrahir · 2022-06-03T11:11:58Z

src/components/figures/chart/chartJs/chartjs.ts

+  get chartRuntime(): ChartJSRuntime {
+    const runtime = this.env.model.getters.getChartRuntime(this.props.figureId);
+    if (!("type" in runtime)) {
+      throw new Error("Unsupported chart runtime");


don't have a suggetion but I don't like that we rely on the presence of a key to decide that, specifically this key. This means taht type exists on all ChartDefinition and all ChartConfiguration but the one of gauge (and potentially the ones from Odoo but I think they'll be chartJs as well)

I do not have a better solution, actually this chart is instantiate based on a matching so...

src/components/figures/chart/chartJs/chartjs.ts

src/components/side_panel/chart/main_chart_panel/main_chart_panel.ts

rrahir · 2022-06-07T07:21:04Z

src/helpers/charts/chart_common.ts

+}
+
+const GraphColors = [
+  // the same colors as those used in odoo reporting


Unrelated but I am again surprised that a graph color does not match its range in the seletction input).

src/components/side_panel/chart/main_chart_panel/main_chart_panel.xml

src/helpers/charts/chart_common.ts

LucasLefevre

some useless key checks and useless if statements

src/helpers/charts/gauge_chart.ts

src/helpers/charts/scorecard_chart.ts

src/helpers/charts/chart_common.ts

LucasLefevre

good opportunity to add readonly everywhere for new/moved types

LucasLefevre · 2022-06-07T14:50:17Z

src/types/chart/gauge_chart.ts

+  rangeMin: string;
+  rangeMax: string;


could they be parsed to numbers earlier to avoid casting to number basically everywhere they are used ?

src/helpers/charts/gauge_chart.ts

No need to pass the array of ranges and the index, just give the range. Also, sheetId is useless since it's already in the range closes #1402 Signed-off-by: Pierre Rousseau (pro) <[email protected]>

Add scorecard chart ------------------- Add "Scorecard" chart type. Create new getters in chart.ts and evaluation_chart.ts specific for this type of chart. The chart itself is not a ChartJS chart, but is a standard Component with hand-written logic for scaling the font sizes. Some work should be done at a latter date in the chart plugins to better handle all types of chart and avoid the repeated if/else. Add gauge chart --------------- Gauge chart based on "chartjs-gauge" library Revamp chart management ----------------------- Since the addition of scorecard and gauge chart, it becomes a mess to manage different types of charts. There is a duplication of many concepts, like getters (getBasicChartDefinition, getGaugeChartDefinition, ...), local state (charts, scorecard, gauge), ... In addition, it was not possible to add a new type of chart from outside o-spreadsheet (for example add Odoo charts). This commit introduce something similar to cellFactory => a chart Factory. Based on the type of the chart in the definition of a chart, an instance of a Chart is created and stored in the local state of chart core plugin. A Chart is an instance of a class (which inherits from AbstractChart), which implements some functions that will be called from the plugin. For each Chart class, a Chart Runtime class should be created to compute the runtime data needed by the components. A Side panel should also be created (a config panel and a design panel). Odoo task 2796685 Co-authored-by: Adrien Minne <[email protected]> Co-authored-by: Alexis Lacroix <[email protected]>

pro-odoo · 2022-06-08T10:55:03Z

Will be done here: #1393

pro-odoo force-pushed the master-poc-chart-pro branch from 54f78c0 to 1633074 Compare May 25, 2022 08:04

pro-odoo changed the base branch from master to master-chart-add-gauge-chart-laa May 25, 2022 08:15

pro-odoo force-pushed the master-poc-chart-pro branch 3 times, most recently from 9de8fa1 to d49c165 Compare May 31, 2022 10:33

pro-odoo changed the title ~~[POC] Charts~~ [REF] charts: revamp chart management May 31, 2022

pro-odoo force-pushed the master-poc-chart-pro branch from d49c165 to e2bb14e Compare May 31, 2022 11:35

pro-odoo mentioned this pull request May 31, 2022

Scorecard + Gauge + REF #1393

Closed

pro-odoo force-pushed the master-poc-chart-pro branch from e2bb14e to 1c6f829 Compare May 31, 2022 11:48

LucasLefevre reviewed Jun 2, 2022

View reviewed changes

hokolomopo reviewed Jun 2, 2022

View reviewed changes

pro-odoo force-pushed the master-poc-chart-pro branch from eed9c64 to 0a58519 Compare June 3, 2022 06:01

pro-odoo force-pushed the master-chart-add-gauge-chart-laa branch from 38bef89 to 895f021 Compare June 3, 2022 06:02

pro-odoo force-pushed the master-poc-chart-pro branch from 0a58519 to e159fd7 Compare June 3, 2022 06:03

pro-odoo force-pushed the master-chart-add-gauge-chart-laa branch from 895f021 to b544dfe Compare June 3, 2022 07:13

pro-odoo force-pushed the master-poc-chart-pro branch from e159fd7 to 4dffb5d Compare June 3, 2022 07:21

LucasLefevre reviewed Jun 7, 2022

View reviewed changes

[FIX] menu: bad CSS for text overflow

b386f08

The CSS to transform text overflow in menus to ellipsis wasn't working. The sub-menu arrow was also not displayed if the name of the menu was too long Odoo task 2877337 closes #1403 Signed-off-by: Lucas Lefèvre (lul) <[email protected]>

rrahir requested changes Jun 7, 2022

View reviewed changes

LucasLefevre reviewed Jun 7, 2022

View reviewed changes

src/helpers/charts/chart_common.ts Outdated Show resolved Hide resolved

LucasLefevre reviewed Jun 7, 2022

View reviewed changes

[REF] evaluation: simplify reference evaluation functions

c767758

No need to pass the array of ranges and the index, just give the range. Also, sheetId is useless since it's already in the range closes #1402 Signed-off-by: Pierre Rousseau (pro) <[email protected]>

LucasLefevre force-pushed the master-poc-chart-pro branch 2 times, most recently from 29c01a4 to edfcee6 Compare June 8, 2022 10:26

pro-odoo force-pushed the master-poc-chart-pro branch from edfcee6 to 503c0a9 Compare June 8, 2022 10:36

pro-odoo closed this Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REF] charts: revamp chart management #1373

[REF] charts: revamp chart management #1373

pro-odoo commented May 20, 2022 •

edited by rrahir

Loading

robodoo commented May 20, 2022

pro-odoo commented May 31, 2022

LucasLefevre left a comment

hokolomopo left a comment

rrahir commented Jun 7, 2022

LucasLefevre left a comment

rrahir left a comment •

edited

Loading

rrahir Jun 3, 2022

pro-odoo Jun 7, 2022

rrahir Jun 7, 2022

LucasLefevre left a comment

LucasLefevre left a comment

LucasLefevre Jun 7, 2022

pro-odoo commented Jun 8, 2022

[REF] charts: revamp chart management #1373

[REF] charts: revamp chart management #1373

Conversation

pro-odoo commented May 20, 2022 • edited by rrahir Loading

Description:

review checklist

robodoo commented May 20, 2022

pro-odoo commented May 31, 2022

LucasLefevre left a comment

Choose a reason for hiding this comment

hokolomopo left a comment

Choose a reason for hiding this comment

rrahir commented Jun 7, 2022

LucasLefevre left a comment

Choose a reason for hiding this comment

rrahir left a comment • edited Loading

Choose a reason for hiding this comment

rrahir Jun 3, 2022

Choose a reason for hiding this comment

pro-odoo Jun 7, 2022

Choose a reason for hiding this comment

rrahir Jun 7, 2022

Choose a reason for hiding this comment

LucasLefevre left a comment

Choose a reason for hiding this comment

LucasLefevre left a comment

Choose a reason for hiding this comment

LucasLefevre Jun 7, 2022

Choose a reason for hiding this comment

pro-odoo commented Jun 8, 2022

pro-odoo commented May 20, 2022 •

edited by rrahir

Loading

rrahir left a comment •

edited

Loading