-
-
Notifications
You must be signed in to change notification settings - Fork 384
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: new Nutri-Score v2 2023 knowledge panels #9689
Conversation
|
There is now an almost finished version on https://world.openfoodfacts.dev The new Nutri-Score 2023 have not been recomputed on the dev server, so edit and save the product to see the changes. @teolemon Note that the new panels are only served to admins, but it seems the app is not authenticating when reading products, so it doesn't get the new Nutri-Score panel. |
@teolemon I can't replicate this, which browser / settings / zoom / resolution etc. are you using? This is what I have in Chrome and Firefox, and it works with all resolutions / zoom levels I tried. |
@teolemon Well it's a link, not a button. There's currently no concept of link buttons in knowledge panels. We could add one, but it would have to be supported in the app as well. |
Well there's no protein subpanel if we don't count them.
I think it's better as-is, less text to read. This is much less useful than the visualization of the points for each component, so we can keep at the end. The numeric score is more for debugging, it's not directly interpretable. |
Generally speaking, it's still a bit messy, my suggestion above are also around more structure, and a cleaner panel |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9689 +/- ##
==========================================
+ Coverage 44.17% 44.32% +0.14%
==========================================
Files 65 65
Lines 20557 20638 +81
Branches 4966 4979 +13
==========================================
+ Hits 9082 9147 +65
- Misses 10294 10321 +27
+ Partials 1181 1170 -11 ☔ View full report in Codecov by Sentry. |
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 this move in the right direction !
Two blockers and some more minor comments, otherwise LGTM.
lib/ProductOpener/KnowledgePanels.pm
Outdated
} | ||
$panel_data_ref->{subtitle} = lang_in_other_lc($target_lc, | ||
"attribute_nutriscore_" . $panel_data_ref->{nutriscore_grade} . "_description_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.
Aren't we missing a else
here ? (otherwise you are over writing the subtitle attribute defined just above)
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.
Good point, fixing.
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 putting in an adequate sub-folder.
|
||
my @files = glob "$dir/nutriscore-*.svg"; | ||
|
||
foreach my $lc ("en", "fr") { |
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.
Only french and english ?
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.
Currently yes.
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.
For generated images, we could also consider not putting them in the git, but putting them in a data folder ?
We could also use javascript with gulp for this kind of things (so that files ends in the dist folders, and developer experience is still easy).
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 a good idea. Maybe we can do it later, especially if we add logos in more languages.
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.
Is it normal to have a serif-font for "New formula" ? (seems a bit less integrated to me).
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 a sans-serif font (the one we use for "Nutri-Score"). In this file we keep the text as text, so that we can easily edit it, and add new languages. Then it has to be converted to paths and added to the .pl script.
<tspan
sodipodi:role="line"
id="tspan18501"
x="39.753796"
y="152.5134"
style="font-style:normal;font-variant:normal;font-weight:800;font-stretch:normal;font-family:Raleway;-inkscape-font-specification:'Raleway Ultra-Bold';fill:#ffffff;stroke-width:0.530607">NEW FORMULA</tspan></text>
@@ -106,8 +106,30 @@ <h5>[% panel.title_element.subtitle %]</h5> | |||
<div class="large-8 small-12 columns"> | |||
[% END %] | |||
|
|||
[% IF panel_group_element.icon_url.defined %] | |||
<img src="[% panel_group_element.icon_url %]" | |||
style="[% IF panel_group_element.icon_size == 'very_small' %]height: 24px;[% ELSIF panel_group_element.icon_size == 'small' %]height:36px;[% ELSE %]height:72px;[% END %]float:left;margin-right:1rem;" |
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.
couldn't we factor this kind of logic in a function ?
like choose_size(panel_group_element.icon_size, "24px", "36px", "72px")
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 an option. Or in that case it might be simpler to just create CSS classes, and make the size part of the class name.
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.
Filed #9752 so that we can take care of it later.
Co-authored-by: Alex Garel <[email protected]>
Co-authored-by: Alex Garel <[email protected]>
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Initially I wanted to use modified nutrition traffic lights as sub panels of the Nutri-Score, in order to explain the points, but the scales really don't match between the UK traffic lights and the new Nutri-Score thresholds. It's very difficult as some nutrients have a maximum of very few points (e.g. 4, 5 or 6 for positive points), while some others can be 10, 15 or 20 (for salt).
I started a table to compare the Nutri-Score thresholds of the traffic lights points if you want to take a look:
https://docs.google.com/spreadsheets/d/16wzjt5yBghmhaA1Ztrh8Rq1N1Esc8br6Wl6mzSuVD-4/edit#gid=0
So instead I use squares so that it's easy to see at a glance what contributes to a good or bad Nutri-Score, added to the design @teolemon created on mockups on Figma: https://www.figma.com/file/nFMjewFAOa8c4ahtob7CAB/Mobile-App-Design-(Quentin)?type=design&node-id=4189-13160&mode=design