Skip to content
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

[MERGE] *: Improve graph view, replace nvd3 with Chart.js #31766

Closed

Conversation

Projects
None yet
6 participants
@Polymorphe57
Copy link
Contributor

commented Mar 12, 2019

The present work is part of an ongoing plan aiming to refactor/improve the reporting views (in particular the graph view).

The most notable changes are

  • better support of comparison mode (comparison of different time periods)
  • better tooltips and legend (content/style)

It has also been decided to use Chart.js instead of nvd3 to draw charts.
The main reasons are that Chart.js is better maintained/designed and offers many interesting features.

Thus we have rewritten all the code depending on nvd3, adapt it to the new library, and remove completely the nvd3 library and the references to it.

Task Ids: 1911201, 1946138

@robodoo robodoo added the CI 🤖 label Mar 12, 2019

@C3POdoo C3POdoo added the RD label Mar 12, 2019

@robodoo robodoo removed the CI 🤖 label Mar 12, 2019

@Polymorphe57 Polymorphe57 force-pushed the odoo-dev:master-graph-view-chartjs-dam branch Mar 12, 2019

@Yenthe666

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

@Polymorphe57 I hope I can ask you a question. I see you're adding chartjs (https://www.chartjs.org/). Does this mean we'll get chartjs available by default in V13? Along with nvd3?

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 12, 2019

addons/web/static/src/js/views/graph/graph_renderer.js Outdated
if (_.contains(this.colors.concat(['#d3d3d3']), color)) {
return this._getRandomColor(index);
}
this.colors.push(color);

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Mar 12, 2019

Contributor

remove this?

addons/web/static/src/js/views/graph/graph_renderer.js Outdated
/**
* @private
*/
_prepareCanvas: function () {

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Mar 12, 2019

Contributor

probably no need for a function here

addons/web/static/src/js/views/graph/graph_renderer.js Outdated
@@ -123,474 +576,224 @@ return AbstractRenderer.extend({
// happens typically after an update), otherwise, it will be
// rendered when the widget will be attached to the DOM (see
// 'on_attach_callback')
this._renderGraph();
this.$el.empty();

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Mar 12, 2019

Contributor

empty once

addons/web/static/src/js/views/graph/graph_renderer.js Outdated
* on the datasets origins. Default is the constant function _ => labels.length.
* @returns {Object} the paramater data used to instatiate the chart.
*/
_prepareData: function (dataPoints, getLabel, getDatasetLabel, getDatasetDataLength) {

This comment has been minimized.

Copy link
@aab-odoo

aab-odoo Mar 12, 2019

Contributor

simplify API

@robodoo robodoo added the CI 🤖 label Mar 12, 2019

@Polymorphe57

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@Polymorphe57 I hope I can ask you a question. I see you're adding chartjs (https://www.chartjs.org/). Does this mean we'll get chartjs available by default in V13? Along with nvd3?

Actually, we plan to remove completely D3 and nvd3 and use Chart.js everywhere (note that the code is still in development). The library will be loaded when needed.

@Yenthe666

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Niceee, that is a huge change and I can already see the benefits in that. Good luck!

@Polymorphe57 Polymorphe57 force-pushed the odoo-dev:master-graph-view-chartjs-dam branch to 4a9d417 Mar 14, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 14, 2019

@Polymorphe57 Polymorphe57 force-pushed the odoo-dev:master-graph-view-chartjs-dam branch from 4a9d417 to a8bf33c Mar 20, 2019

@robodoo robodoo removed the CI 🤖 label Mar 20, 2019

@Polymorphe57 Polymorphe57 force-pushed the odoo-dev:master-graph-view-chartjs-dam branch from a8bf33c to e63a87d Mar 22, 2019

@robodoo robodoo added the CI 🤖 label Mar 22, 2019

@Polymorphe57 Polymorphe57 force-pushed the odoo-dev:master-graph-view-chartjs-dam branch from e63a87d to 3b472c8 Mar 22, 2019

@robodoo robodoo removed the CI 🤖 label Mar 22, 2019

@Polymorphe57 Polymorphe57 force-pushed the odoo-dev:master-graph-view-chartjs-dam branch 3 times, most recently from 8140dd2 to 6dbcb92 Mar 22, 2019

@robodoo robodoo added the CI 🤖 label Mar 22, 2019

@Polymorphe57 Polymorphe57 force-pushed the odoo-dev:master-graph-view-chartjs-dam branch from 6dbcb92 to 6780a4b Mar 25, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 25, 2019

Show resolved Hide resolved addons/web/static/src/js/core/data_comparison_utils.js Outdated
*
* @return {number}
*/
referenceIndex: function () {

This comment has been minimized.

Copy link
@ged-odoo

ged-odoo Mar 26, 2019

Contributor

this is overkill. you can remove this function in my opinion

* @param {number} index
* @return {string}
*/
representative: function (dateClass, index) {

This comment has been minimized.

Copy link
@ged-odoo

ged-odoo Mar 26, 2019

Contributor

are you aware that if index is 0, it will fall back on referenceindex?

* index.
*
* @param {number} dateClass
* @param {number} index

This comment has been minimized.

Copy link
@ged-odoo

ged-odoo Mar 26, 2019

Contributor

should be marked optional

Show resolved Hide resolved addons/web/static/src/js/views/graph/graph_controller.js
Show resolved Hide resolved addons/web/static/src/js/views/graph/graph_controller.js Outdated
Show resolved Hide resolved addons/web/static/src/js/views/graph/graph_model.js Outdated
* @return {string}
*/
representative: function (dateClass, index) {
index = index || this.referenceIndex();

This comment has been minimized.

Copy link
@ged-odoo

ged-odoo Mar 26, 2019

Contributor
Suggested change
index = index || this.referenceIndex();
index = index === undefined ? this._referenceIndex : index;
Show resolved Hide resolved addons/web/static/src/js/views/graph/graph_renderer.js Outdated
var FORMAT_OPTIONS = {
// allow to decide if utils.human_number should be used
humanReadable: function (value) {
return Math.abs(value) >= 1000;

This comment has been minimized.

Copy link
@ged-odoo

ged-odoo Mar 26, 2019

Contributor

why not this?

Suggested change
return Math.abs(value) >= 1000;
return value >= 1000;

This comment has been minimized.

Copy link
@ged-odoo

ged-odoo Mar 26, 2019

Contributor

ignore this

@robodoo robodoo added the CI 🤖 label Mar 26, 2019

@robodoo robodoo added the r+ 👌 label Apr 1, 2019

@Polymorphe57 Polymorphe57 force-pushed the odoo-dev:master-graph-view-chartjs-dam branch from e1e5718 to 6516458 Apr 1, 2019

@robodoo robodoo removed the r+ 👌 label Apr 1, 2019

Polymorphe57 and others added some commits Mar 12, 2019

[FIX] web: correctly update this.widgets
Old subwidgets of a widget inhereriting from BasicRender were not
correctly destroyed. This was the cause of various problems when
cycling over this.widgets.
[FIX] web: correct quarter format in mockReadGroup
Before this commit the value of a field (of date/datetime type)
used as a groupby was not correctly formatted in the test environment
when the granularity was 'quarter'. This commit fixes that situation.
[FIX] web: on_attach_callback of form renderer widgets
Before this commit, the 'on_attach_callback' method of a subwidget of
the form renderer would not be called when the renderer renders
itself but is already in the dom. This can cause problems when a
subwidget has to know if it is in the dom for its own rendering.
This commit fixes that situation.
[IMP] web: add library Chart.js
This commit makes possible to use the Chart.js library to create
various charts.

Task ID: 1946138
[IMP] web: graph view improvements
This commit extends the support of comparison mode for the graph view
and refine the tooltips and rendering in line mode.
The graph view code has also been slightly refactored.

Task ID: 1946138
[IMP] web: use Chart.js in graph view
It has been decided to use Chart.js instead of nvd3 to render charts.
In this commit the graph view has been largely rewritten to benefit
from the options offered by Chart.js.

Task ID: 1946138
[IMP] website: use Chart.js in community dashboard
This commit is one part of the task 'removing nvd3'.
We use now Chart.js to draw charts.

Task ID: 1946138
[REF] web_kanban_gauge: reimplement gauge widget with Chart.js
This commit is one part of the task 'removing nvd3'.
We use now Chart.js to draw charts.

Task ID: 1946138
[REF] reimplement journal dashboard widget with Chart.js
This commit is one part of the task 'removing nvd3'.
We use now Chart.js to draw charts.

Task ID: 1946138
[FIX] web: call super in AbstractAction willStart
The commit odoo/enterprise@19a144d has
modified how libraries are loaded at several places and has
introduced a bug in sale_subscription_dashboard where the
libraries are no longer loaded at all. We fix that situation
by calling super in the willStart method of AbstractAction
that inherits from Widget. Abstract actions can now also
benefit from the mechanism present in Widget willStart.
[IMP] web: remove nvd3 library
It has been decided to use Chart.js instead of nvd3 to render charts.
Now that nvd3 is no more in use, we remove it.

Task ID: 1946138
[IMP] sales_team: remove useless css rules
This commit is one part of the task 'removing nvd3'.
We use now Chart.js to draw charts. This makes old CSS rules
related to nvd3 useless.

Task ID: 1946138
[IMP] survey: use Chart.js instead of nvd3 in survey results
This commit is one part of the task 'removing nvd3'.
We use now Chart.js to draw charts.

Task ID: 1946138
[IMP] *: use Chart.js in website_links/website_sale_link_tracker.
This commit is one part of the task 'removing nvd3'.
We use now Chart.js to draw charts.

Task ID: 1946138

@Polymorphe57 Polymorphe57 force-pushed the odoo-dev:master-graph-view-chartjs-dam branch from 6516458 to 4d8cfd1 Apr 1, 2019

@ged-odoo

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

robodoo r+

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward
@ged-odoo

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

robodoo rebase-merge

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Merge method set to rebase and merge, using the PR as merge commit message

robodoo added a commit that referenced this pull request Apr 2, 2019

[MERGE] *: Improve graph view, replace nvd3 with Chart.js
The present work is part of an ongoing plan aiming to refactor/improve the reporting views (in particular the graph view).

The most notable changes are
- better support of comparison mode (comparison of different time periods)
- better tooltips and legend (content/style)

It has also been decided to use Chart.js instead of nvd3 to draw charts.
The main reasons are that Chart.js is better maintained/designed and offers many interesting features.

Thus we have rewritten all the code depending on nvd3, adapt it to the new library, and remove completely the nvd3 library and the references to it.

Task Ids: 1911201, 1946138

closes #31766

Signed-off-by: Géry Debongnie (ged) <ged@openerp.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.