-
-
Notifications
You must be signed in to change notification settings - Fork 407
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: Nutri Score Nutriments with an asterisk #8205
fix: Nutri Score Nutriments with an asterisk #8205
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @MonalikaPatnaik
fix some formatting issue though
templates/web/pages/product_edit/product_edit_form_display.tt.html
Outdated
Show resolved
Hide resolved
@@ -207,7 +207,11 @@ <h1>[% title %]</h1> | |||
<td> | |||
<!--label starts--> | |||
[% IF nutriment.name.defined %] | |||
<label class="nutriment_label" for="nutriment_[% nutriment.enid %]">[% nutriment.prefix %][% nutriment.name %]</label> | |||
[% IF nutriment.nid == 'fat' || nutriment.nid == 'saturated-fat' || nutriment.nid == 'salt'|| nutriment.nid =='sugars' %] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
energy in kJ and fibers are also used for the Nutri-Score.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MonalikaPatnaik , thanks for the PR!
If we put a
*
, we need to explicitly tell users what this*
means, otherwise there is no way to guess. Could you add a string after the table that states "*
: used to compute the Nutri-Score" or similar?That string would need to be translatable, so you would need to use the lang() function, and add it in the translation files: po/common/common.pot + po/common/en.po
Thanks for the review..will make necessary changes :)
Hi @MonalikaPatnaik , thanks for the PR! If we put a That string would need to be translatable, so you would need to use the lang() function, and add it in the translation files: po/common/common.pot + po/common/en.po |
…html Co-authored-by: jnsereko <58003327+jnsereko@users.noreply.github.com>
Co-authored-by: Stéphane Gigandet <stephane@openfoodfacts.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested it out 👍
you should probably remove the extra changes that got staged
Okay..Thanks<3 |
Heyy..done the req changes..check once!! Should I squash the commits into one! |
@@ -25,6 +25,7 @@ <h1>[% title %]</h1> | |||
[% END %] | |||
|
|||
[% IF errors_index >=0 %] | |||
<p>[% lang("correct_the_following_errors") %]</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string has been added twice
@@ -59,6 +60,9 @@ <h1>[% title %]</h1> | |||
<p id="barcode_paragraph"> [% lang("barcode") %][% sep %]: | |||
<span id="barcode" property="food:code" itemprop="gtin13" style="speak-as:digits;">[% code %]</span> | |||
</p> | |||
<p id="barcode_paragraph"> [% lang("barcode") %][% sep %]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, strings have been added twice
@@ -209,7 +213,11 @@ <h1>[% title %]</h1> | |||
<td> | |||
<!--label starts--> | |||
[% IF nutriment.name.defined %] | |||
[% IF nutriment.nid == 'energy-kj' || nutriment.nid == 'saturated-fat' || nutriment.nid == 'salt'|| nutriment.nid =='sugars' || nutriment.nid == 'fiber' %] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's missing fat (needed for some food categories) and proteins
taxonomies/nutrient_levels.txt
Outdated
@@ -117,7 +117,7 @@ nb:Fett i liten kvantitet | |||
nd:Fat in low quantity | |||
ne:Fat in low quantity | |||
ng:Fat in low quantity | |||
nl:Vetten in kleine hoeveelheid | |||
nl:Vetten in low quantity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove the changes in this file? they are unrelated...
you can do "git checkout taxonomies/nutrient_levels.txt" to get back the original file
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making the changes to it Monalika.
Like I said in the previous comment, you should remove the unrelated changed taxonomy file.
Yup..I have discarded the changes and commited the previous state of the file only |
I don't think it should be a problem. but it's still showing up in your changed file diff even when there is no apparent change. Edit: you might have to do a force-push to get that file out from changed files. but I really don't think it matters here since there seems to be no apparent change. |
Above is just a nit-pick. This should be good to merge I believe. Thank you so much! 💯 |
Okay..I'll keep this in mind..if u say I could squash all my commits into one having the necessary changed files only! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you for addressing all the comments @MonalikaPatnaik!
What
So, to calculate nutriscore-- fats,saturated-fats, sugar and salt are the required nutriments to be considered, so I have made necessary changes in the code to show these nutriments with an asterisk in the nutrition table:)
Screenshot
Related issue(s) and discussion