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

Fix/96 plot in report #98

Merged
merged 4 commits into from
Mar 21, 2019
Merged

Fix/96 plot in report #98

merged 4 commits into from
Mar 21, 2019

Conversation

fvitalini
Copy link
Contributor

@fvitalini fvitalini commented Mar 12, 2019

@fvitalini fvitalini changed the base branch from master to develop March 12, 2019 13:02
@riccardoporreca
Copy link
Member

riccardoporreca commented Mar 20, 2019

@fvitalini, I think we should be more explicit about what was done here.

The main driver was fixing bug #96.

On top of this, plot labels were changed from Occupational/Private Pension to Occupational/Private Fund in both the app and report, which were aligned. The renaming was probably wanted (@gabrielfoix?), but it does not quite belong to the bugifx and its branch (strictly speaking).

What is definitely missing is mentioning bugfix #96 in NEWS.md, which should rather read as something like

* Fixed missing bar-plot in the PDF report (#6).
* Reviewed plots labels.

Actually, naming probably deserves its own issue, since we are not very consistent between plots, table, text in the report. I opened a dedicated issue #103, we might want to revert the labels change done here and delegate it to it.

@fvitalini
Copy link
Contributor Author

@fvitalini, I think we should be more explicit about what was done here.

The main driver was fixing bug #96.

On top of this, plot labels were changed from Occupational/Private Pension to Occupational/Private Fund in both the app and report, which were aligned. The renaming was probably wanted (@gabrielfoix?), but it does not quite belong to the bugifx and its branch (strictly speaking).

What is definitely missing is mentioning bugfix #96 in NEWS.md, which should rather read as something like

* Fixed missing bar-plot in the PDF report (#6).
* Reviewed plots labels.

Actually, naming probably deserves its own issue, since we are not very consistent between plots, table, text in the report. I opened a dedicated issue #103, we might want to revert the labels change done here and delegate it to it.

@riccardoporreca In commit 32dadde I have updated the NEWS.md according to your suggestion.

@riccardoporreca riccardoporreca self-requested a review March 21, 2019 16:45
Copy link
Member

@riccardoporreca riccardoporreca left a comment

Choose a reason for hiding this comment

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

As clarified with @fvitalini and @gabrielfoix, the pull request also covers, on top of bugfix #96, the renaming the plot legends to make them consistent with the report tables. News.md updated accordingly to be more explicit (dba29da).

@riccardoporreca riccardoporreca merged commit d8f2f3d into develop Mar 21, 2019
@riccardoporreca riccardoporreca deleted the fix/96-plot-in-report branch March 21, 2019 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants