-
-
Notifications
You must be signed in to change notification settings - Fork 408
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: ingredients knowledge panels #10904
Conversation
/update_tests_results |
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.
LGTM but some comments and maybe a mistake ?
[% IF panel.ingredient_description OR panel.wikipedia_abstract %] | ||
"expanded": false, | ||
[% ELSE %] | ||
"expanded": false, |
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 is strange !
Shouldn't the second expanded be true ?
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 temporary, in fact what we really want is to make the first one (no description) not expandable. But currently the non expandable panels are displayed in a very different way (at least on the web, I haven't checked mobile). So we will need a better way.
"expanded": false, | ||
[% END %] | ||
"title_element": { | ||
// Note: the app displays line feeds as line feeds... |
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.
// Note: the app displays line feeds as line feeds... | |
// Note: the app displays line feeds as line feeds... | |
// so we have to keep whole title expression on one line ! |
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 just added a double backtick function so that we can have things on multiple lines that are then converted to one line without linebreaks.
[% END %] | ||
"title_element": { | ||
// Note: the app displays line feeds as line feeds... | ||
"title": ` [% dash = "-" %] [% dash.repeat(panel.level) %] [% display_taxonomy_tag_name("ingredients", panel.ingredient_id) %] [% IF panel.ingredient.percent.defined %] ([% round(panel.ingredient.percent) %]%) [% ELSIF panel.ingredient.percent_estimate.defined %] ([% round(panel.ingredient.percent_estimate) %]% [% lang("estimate") %]) [% END %] `, |
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't we eventually deport this computation in the ingredient panel creation ?
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.
Well the idea of using Template::Toolkit was that people could change the layout / design decisions from the panels, without having to change the Perl code. So that's why I try to put things that are more design things in the template. Here I tried to show the sub-ingredients, and I'm just pushing dashes in front, which is not very clear (but it's something..). There may be other better ways to do that.
@@ -55,6 +55,15 @@ | |||
`, | |||
}, | |||
}, | |||
[% END %] | |||
[% END %] | |||
[% IF 1 %] |
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 should be if we have feature ingredient right ?
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.
ah it should be if we have an ingredients_list panel, but the condition I put before didn't work, so I just put 1. I'll change that.
Co-authored-by: Alex Garel <alex@openfoodfacts.org>
Co-authored-by: Alex Garel <alex@openfoodfacts.org>
/update_tests_results |
@stephanegigandet some feedback from this morning's battle testing.
|
Thanks for the feedback @teolemon
I'm not sure what you mean by "it should be in the card", if we only see the % of the ingredient when we click on it, it's less useful than being able to see the % for all ingredients. That's one of the added value compared to the original list of ingredients from the package: Not sure either about the localization, today we display "Potato (6% estimate)" in English and "Pomme de terre (6% estimation)" in French, so it is localized. Maybe "estimate" takes too much room? We could do ~6%, but it's less explicit.
We display the text ingredient list in the user's language if we do have it, and we display the ingredients panels in the user language in all cases. I would think it's more useful that way?
Yes, this is to convey that there sub ingredients (or sub sub sub ingredients). Not ideal, but better than a completely flat list I guess. We could try to indent more instead of using hyphens, but we would need to extend a bit the knowledge panels grammar. |
|
=head2 convert_multiline_string_to_singleline_without_line_breaks_and_extra_spaces($line) | ||
|
||
Helper function to allow to enter multiline strings in JSON templates. | ||
The function converts the multiline string into a single line string. | ||
|
||
Line breaks are converted to spaces, and multiple spaces are converted to a single space. | ||
|
||
This function is useful in templates where we use IF statements etc. to generate a single value like a title. | ||
|
||
=cut | ||
|
||
sub convert_multiline_string_to_singleline_without_line_breaks_and_extra_spaces ($line) { |
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 idea :-)
"title_element": { | ||
// Note: the app displays line feeds as line feeds... | ||
"title": ` [% dash = "-" %] [% dash.repeat(panel.level) %] [% display_taxonomy_tag_name("ingredients", panel.ingredient_id) %] [% IF panel.ingredient.percent.defined %] ([% round(panel.ingredient.percent) %]%) [% ELSIF panel.ingredient.percent_estimate.defined %] ([% round(panel.ingredient.percent_estimate) %]% [% lang("estimate") %]) [% END %] `, | ||
// double backticks: convert to a single line without newlines and extra spaces |
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.
😲 cool !
…dfacts-server into ingredients-panels
/update_tests_results |
|
@stephanegigandet some merge conflicts. |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #10904 +/- ##
==========================================
- Coverage 49.33% 49.27% -0.06%
==========================================
Files 79 80 +1
Lines 22510 22563 +53
Branches 5387 5393 +6
==========================================
+ Hits 11105 11118 +13
- Misses 10045 10085 +40
Partials 1360 1360 ☔ View full report in Codecov by Sentry. |
|
🤖 I have created a release *beep* *boop* --- ## [2.57.0](v2.56.0...v2.57.0) (2025-02-04) ### Features * Add security.txt ([#11290](#11290)) ([2ce94d8](2ce94d8)) * enable nutripatrol panel on mobile ([#11323](#11323)) ([741d414](741d414)) * ingredients knowledge panels ([#10904](#10904)) ([2443694](2443694)) ### Bug Fixes * better analysis of "oil (rapeseed, something unrecognized)" + separation of additive class + additive ([#11251](#11251)) ([fde3287](fde3287)) * FLAVOR_NAME is openproductsfacts ([#11312](#11312)) ([c4fa828](c4fa828)) * Import format_subdomain ([#11288](#11288)) ([930dc47](930dc47)) * Open Beauty Facts string fixes ([92de3be](92de3be)) * Open Beauty Facts string fixes ([#11286](#11286)) ([92de3be](92de3be)) * Open Products Facts lang fixes ([b8651b8](b8651b8)) * Open Products Facts lang fixes ([#11285](#11285)) ([b8651b8](b8651b8)) * separate granulated sugars from white sugars ([#11325](#11325)) ([cfb5fd5](cfb5fd5)) * set APACHE_ENVVARS=/etc/apache2/off-envvars + doc precision ([#11279](#11279)) ([da7185a](da7185a)) * Update `green-score-f.svg` (white background) ([#11314](#11314)) ([d57e2de](d57e2de)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR adds knowledge panels for ingredients, to list each ingredient below the ingredient list.
Each ingredient panel can be opened to get a description from: https://github.com/openfoodfacts/openfoodfacts-web/tree/main/knowledge_panels
Known issue: most ingredients do not currently have a description, they contain an empty panel.