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.

Sign up for GitHub

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

Jump to bottom

[13.0][MIG] web_pivot_computed_measure: Migration to 13.0 #1746

Merged
merged 6 commits into from
Dec 1, 2020
Merged

[13.0][MIG] web_pivot_computed_measure: Migration to 13.0 #1746

merged 6 commits into from
Dec 1, 2020

Conversation

ernestotejeda
Copy link
Member

cc @Tecnativa TT25958

Main changes:

  • Adapt the javascript code to the new data structure used to manage the data to be displayed in the pivot view.

Tardo and others added 5 commits November 26, 2020 00:49
@Tardo
[ADD] web_pivot_computed_measure
59e9278
@Tardo
[IMP+FIX] web_pivot_computed_measure: Time Ranges Comparison + Change…
08ff9eb 
… custom_events addition

[IMP] web_pivot_computed_measure: Time Ranges Comparison

[FIX] web_pivot_computed_measure: Change custom_events addition

Previous this commit, the controller events are discarted. With this commit
the existing events are respected
@pedrobaeza
[FIX] web_pivot_computed_measure: Proper trailing line in RST
64f8a6f
@claudiagn
Added translation using Weblate (Spanish)
c77572b
[IMP] web_pivot_computed_measure: black, isort, prettier
19adc59
@pedrobaeza pedrobaeza added this to the 13.0 milestone Nov 26, 2020
pedrobaeza
pedrobaeza approved these changes Nov 26, 2020
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested functionally on runbot and working. Maybe you can put in the same line the label and the dropdowns for saving vertical space.

Tardo
Tardo requested changes Nov 26, 2020
View reviewed changes
Copy link
Member

@Tardo Tardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, update source to use ES6 :)

And take a look at this PR: #1700 maybe need be fixed on 13.0 too

web_pivot_computed_measure/static/src/js/pivot_controller.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_controller.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_controller.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
@ernestotejeda ernestotejeda force-pushed the 13.0-mig-web_pivot_computed_measure branch from 47700fe to 5b83d24 Compare November 27, 2020 18:28
@ernestotejeda
Copy link
Member Author

@Tardo changes done. I applied the changes you proposed and I added #1700 fix in this migration because it is also needed in v13

Tardo
Tardo requested changes Nov 27, 2020
View reviewed changes
Copy link
Member

@Tardo Tardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I see more changes to be made:

  • Review 'let' usage instead of 'var'.
  • Use arrow functions when possible... arrow functions do not have their own "this" value, so we can avoid 'self' usage.

web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_controller.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_controller.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_controller.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_controller.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
web_pivot_computed_measure/static/src/js/pivot_model.js Outdated Show resolved Hide resolved
@ernestotejeda ernestotejeda force-pushed the 13.0-mig-web_pivot_computed_measure branch from 5b83d24 to 6cd4847 Compare November 30, 2020 16:11
@ernestotejeda
Copy link
Member Author

@Tardo Changes done

Tardo
Tardo approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@Tardo Tardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good! Thanks

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-1746-by-pedrobaeza-bump-nobump, awaiting test results.

@pedrobaeza pedrobaeza mentioned this pull request Dec 1, 2020
Closed
41 tasks
@OCA-git-bot OCA-git-bot merged commit 22cd6e2 into OCA:13.0 Dec 1, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at b49602e. Thanks a lot for contributing to OCA. ❤️

@ernestotejeda ernestotejeda mentioned this pull request Dec 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tardo Tardo Tardo approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ernestotejeda @pedrobaeza @OCA-git-bot @Tardo @claudiagn