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

Introduce a flag to display the progress #2847

Merged
merged 1 commit into from
Feb 28, 2018

Conversation

ferblape
Copy link
Contributor

🎩 What? Why?

This PR introduces a flag to enable/disable progress display in the accountability module. This is useful when there's no progress data uploaded, to avoid all the percentages to be 0% and the progress bar be empty.

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG entry

📷 Screenshots (optional)

New setting:

screen shot 2018-02-27 at 15 24 40

UI with the setting enabled:

screen shot 2018-02-27 at 15 26 34

UI with the setting disabled:

screen shot 2018-02-27 at 15 24 58

@ghost ghost assigned ferblape Feb 27, 2018
@ghost ghost added the in-progress label Feb 27, 2018
Copy link
Contributor

@mrcasals mrcasals left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @ferblape! Code looks good to me, but I left a comment regarding some i18n 😄

@ferblape
Copy link
Contributor Author

Thanks @mrcasals but maybe you didn't submit the review, because I can't see the comments :D

@@ -173,6 +173,7 @@ en:
heading_parent_level_results: Name for "Results"
intro: Intro
subcategories_label: Name for "Subcategories"
display_progress_enabled: Display progress enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe leave this as "Display progress"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@mrcasals
Copy link
Contributor

LOL @ferblape sorry 😞 I left the comment now!

@ferblape ferblape force-pushed the feature/2737-flag-control-visualization branch 2 times, most recently from de90e77 to 54f7559 Compare February 28, 2018 06:24
@mrcasals
Copy link
Contributor

@ferblape CI is failing :( ignore the meetings one as it looks like a random failure, but the error in Rubocop is legit (change to_not to not_to)

Please do this and we'll merge it straight away 😄

@ferblape ferblape force-pushed the feature/2737-flag-control-visualization branch from 54f7559 to c891344 Compare February 28, 2018 08:52
@codecov
Copy link

codecov bot commented Feb 28, 2018

Codecov Report

Merging #2847 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2847      +/-   ##
==========================================
+ Coverage   98.73%   98.73%   +<.01%     
==========================================
  Files        1636     1636              
  Lines       38719    38730      +11     
==========================================
+ Hits        38230    38241      +11     
  Misses        489      489

@ferblape
Copy link
Contributor Author

Or even better .to have_no_content, which is faster than .not_to have_content 🚀

@mrcasals
Copy link
Contributor

@ferblape and they behave differently, and the correct usage here is .to have_no_content 😄

@mrcasals
Copy link
Contributor

Argh, conflicts in the CHANGELOG too! 😞

@ferblape ferblape force-pushed the feature/2737-flag-control-visualization branch from c891344 to 87958ad Compare February 28, 2018 09:13
@mrcasals
Copy link
Contributor

@ferblape tests are now failing because the i18n is not normalized (keys must be sorted alphabetically).

you can run i18n-tasks normalize en from the decidim root folder to fix it 😄 (and there are more conflicts on the CHANGELOG 😭 )

@ferblape ferblape force-pushed the feature/2737-flag-control-visualization branch from 87958ad to 943c889 Compare February 28, 2018 09:42
@ferblape
Copy link
Contributor Author

ferblape commented Feb 28, 2018 via email

@mrcasals mrcasals merged commit af91fdf into master Feb 28, 2018
@ghost ghost removed the in-progress label Feb 28, 2018
@mrcasals mrcasals deleted the feature/2737-flag-control-visualization branch February 28, 2018 11:08
@mrcasals
Copy link
Contributor

Yay, thanks! 🎉

@ferblape
Copy link
Contributor Author

👯

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.

3 participants