-
-
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
feat: Show EcoScore attribute panel for world. Fixes #7378 #7913
Conversation
po/common/common.pot
Outdated
@@ -5747,6 +5747,14 @@ msgctxt "ecoscore_warning_international" | |||
msgid "The Eco-Score was initially developped for France and it is being extended to other European countries. The Eco-Score formula is subject to change as it is regularly improved to make it more precise and better suited to each country." | |||
msgstr "The Eco-Score was initially developped for France and it is being extended to other European countries. The Eco-Score formula is subject to change as it is regularly improved to make it more precise and better suited to each country." | |||
|
|||
msgctxt "ecoscore_warning_transportation" |
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.
I'm thinking that it would be better to have different messages for the 2 cases: not having a country selected, and having a country selected that we don't support yet.
The messages could be something like:
short: The impact of transportation is not included.
long on world: Select a country in order to include the impact of transportation.
long on a not supported country: The impact of transportation to your country is currently unknown.
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.
Do we need to say "...the full impact of transportation..." as some impacts are still shown even for world?
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.
Sounds good, we can say "the full impact"
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!
lib/ProductOpener/Attributes.pm
Outdated
@@ -632,7 +632,9 @@ sub compute_attribute_ecoscore ($product_ref, $target_lc, $target_cc) { | |||
$score = $product_ref->{ecoscore_data}{"scores"}{$target_cc} // 0; | |||
$grade = $product_ref->{ecoscore_data}{"grades"}{$target_cc}; | |||
} | |||
|
|||
if (($target_cc eq "world") or (!defined $product_ref->{ecoscore_data}{"scores"}{$target_cc})) { | |||
$attribute_ref->{missing} = lang_in_other_lc($target_lc, "ecoscore_warning_transportation_short"); |
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.
In fact we use the "missing" field of attributes only when the attribute cannot be computed (e.g. we don't have the ingredients list, so we don't know if there's an allergen).
When we display attributes in the summary, we don't display warnings (like for the Nutri-Score, we don't say that fibers are missing and were not taken into account). We display the warning in the knowledge panel instead.
Otherwise it makes a very big summary:
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.
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.
I'm unclear on what you are saying here - we are already displaying the "Select a country..." warning here. Are you proposing something in addition to that?
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.
@john-gom We can leave the "Select a country" warning as-is.
scss/_product-page.scss
Outdated
@@ -75,6 +75,9 @@ $decalTop:48px; | |||
} | |||
.attr_text { | |||
padding-left:0.5rem; | |||
.attribute_missing::before { |
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.
The attribute missing is not really a warning, it's more an explanation of why an attribute is not computed (and thus shown in grey), so we should not display a warning sign there.
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.
So shall I remove all the changes to the attribute panel?
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.
Yes, I think the attribute panel should stay as it was.
Kudos, SonarCloud Quality Gate passed!
|
Kudos @john-gom ! |
What
Show the EcoScore card even when there is no localised EcoScore. Displays a warning to indicate that transportation impact won't be as accurate as it could be
Screenshot
See Issue #7378
Related issue(s) and discussion