feat: Make attributes clickable (without Provider) #1666

M123-dev · 2022-04-27T11:31:50Z

What

Revision of feat: Make attributes clickable #1654
Fixes: Make attributes tapable in the Summary card #1029
Makes Attributes the ScoreCard (Nutri- / Eco-Score) expandable.
Moved the full knowledge panel page from knowledge_panel_card to knowledge_panel_page
new_product_page moved knowledge panel query from build method to initState + update method.
summary_card created new method openFullKnowledgePanel to open knowledge_panel_page with opening LoadingDialog if the knowledge panels aren't loaded already

codecov-commenter · 2022-04-27T11:34:10Z

Codecov Report

Merging #1666 (c50d4e0) into develop (2ea0da3) will decrease coverage by 0.59%.
The diff coverage is 0.38%.

@@            Coverage Diff             @@
##           develop   #1666      +/-   ##
==========================================
- Coverage     8.86%   8.26%   -0.60%     
==========================================
  Files          161     166       +5     
  Lines         6623    7149     +526     
==========================================
+ Hits           587     591       +4     
- Misses        6036    6558     +522

Impacted Files	Coverage Δ
...h_app/lib/cards/category_cards/abstract_cache.dart	`0.00% <0.00%> (ø)`
...p/lib/cards/category_cards/asset_cache_helper.dart	`0.00% <0.00%> (ø)`
...p/lib/cards/category_cards/raster_async_asset.dart	`0.00% <0.00%> (ø)`
...oth_app/lib/cards/category_cards/raster_cache.dart	`0.00% <0.00%> (ø)`
..._app/lib/cards/category_cards/svg_async_asset.dart	`0.00% <0.00%> (ø)`
...smooth_app/lib/cards/category_cards/svg_cache.dart	`0.00% <0.00%> (ø)`
...t_cards/knowledge_panels/knowledge_panel_card.dart	`0.00% <0.00%> (ø)`
...t_cards/knowledge_panels/knowledge_panel_page.dart	`0.00% <0.00%> (ø)`
...rds/knowledge_panels/knowledge_panels_builder.dart	`2.17% <0.00%> (ø)`
...pp/lib/cards/product_cards/product_title_card.dart	`0.00% <0.00%> (ø)`
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3af05bd...c50d4e0. Read the comment docs.

monsieurtanuki · 2022-04-27T13:36:33Z

@teolemon Now that we download products by (smaller) packs of 25, couldn't we download the knowledge panels directly with the product? It's just a product field, that is not included in the default list in smoothie.
That would save us some tap dancing about refreshing part of the product page, wouldn't it?

jasmeet0817 · 2022-04-27T13:41:46Z

packages/smooth_app/lib/pages/product/new_product_page.dart

@@ -54,6 +53,9 @@ class _ProductPageState extends State<ProductPage> {
    AnalyticsHelper.trackProductPageOpen(
      product: _product,
    );
+    knowledgePanels = KnowledgePanelsQuery(


so this will reissue a backend query, just to confirm, knowledge_panels are not fetched as part of the product query when we scan a product ?

Also, will this be cached ?

@jasmeet0817 The answers are no and no. Good point, cf. #1666 (comment)

packages/smooth_app/lib/pages/product/summary_card.dart

jasmeet0817 · 2022-04-27T14:57:30Z

packages/smooth_app/lib/pages/product/summary_card.dart

+    final KnowledgePanel? knowledgePanel =
+        knowledgePanels?.panelIdToPanelMap[attribute.panelId];
+
+    if (knowledgePanel == null || knowledgePanels == null) {


will kind of suck if the user sees Loading dialog and then the KP is not found. And nothing happens :(

We can of course show a Snackbar or something similar there but I don't think that solves the fundamental problem, combining the query's on the other hand would simplfy everything

jasmeet0817 · 2022-04-27T14:58:44Z

packages/smooth_app/lib/pages/product/summary_card.dart

+    }
+
+    if (!done) {
+      knowledgePanels = await LoadingDialog.run<KnowledgePanels>(


What does this loading dialog look like ? Does it just say "Loading". That might be ok but I'm just checking if we want a custom message ?

The default loading dialog title is Downloading data

M123-dev · 2022-04-27T17:32:55Z

Im definitely in favor of combining the querys, this would simplfy the whole code

M123-dev · 2022-04-28T13:58:35Z

Are we good to merge @monsieurtanuki @jasmeet0817 ?

monsieurtanuki · 2022-04-28T14:09:11Z

Are we good to merge @monsieurtanuki @jasmeet0817 ?

As far as I'm concerned, no. I don't understand why we should keep knowledge panels apart from the other product data, and as @jasmeet0817 pointed out that means that they are not cached in the local database. My suggested solution: put the knowledge panels in the default product fields. Doing so is so much simpler.

jasmeet0817 · 2022-04-28T15:42:47Z

Are we good to merge @monsieurtanuki @jasmeet0817 ?

As far as I'm concerned, no. I don't understand why we should keep knowledge panels apart from the other product data, and as @jasmeet0817 pointed out that means that they are not cached in the local database. My suggested solution: put the knowledge panels in the default product fields. Doing so is so much simpler.

Sorry I don't have laptop access anymore, so I'm texting from phone.

What was the reason for the split of the two api calls? Is it to reduce payload size when scanning? If that's the case then I wonder if there's a way of making a background api call for products on scan to fetch their KPs, otherwise page opening will be slow.

I also think caching is super important.

monsieurtanuki · 2022-04-28T15:56:16Z

@jasmeet0817 I'm planning to build a button that refreshes all the database from the back-end. That's more or less needed by #1651.
With that we can test how dramatic including KPs would be.

monsieurtanuki · 2022-04-28T17:37:03Z

@M123-dev @jasmeet0817 @teolemon I've just tested the size of the results, with 3 barcodes, with the current smoothie fields, and with the current fields plus knowledge panels.
The resulting strings are about 2.3 times bigger with the KP.

I really think we can afford a 2.3 bigger size now that we're more frugal with smaller 25 pack.

barcode	size with KP	(current) size without KP	ratio
`7300400481588`	41511	18425	2.25
`3660140948258`	37691	16090	2.34
`3046920011334`	39251	15188	2.58

For the record, URI for 7300400481588 with knowledge panels:
https://world.openfoodfacts.org/api/v0/product/7300400481588.json?lc=fr&cc=fr&fields=product_name%2Cbrands%2Ccode%2Cnutrition_grade_fr%2Cimage_small_url%2Cimage_front_small_url%2Cimage_front_url%2Cimage_ingredients_url%2Cimage_nutrition_url%2Cimage_packaging_url%2Cselected_images%2Cquantity%2Cserving_size%2Cproduct_quantity%2Cnutriments%2Cnutrient_levels%2Cnutriment_energy_unit%2Cadditives_tags%2Cingredients_analysis_tags%2Cingredients_text%2Clabels_tags%2Clabels_tags_fr%2Cenvironment_impact_level_tags%2Ccategories_tags%2Ccategories_tags_fr%2Clang%2Cattribute_groups%2Cstates_tags%2Cecoscore_data%2Cecoscore_grade%2Cecoscore_score%2Cknowledge_panels

teolemon · 2022-04-28T18:33:55Z

Ok, let's try.

monsieurtanuki · 2022-04-28T18:51:20Z

Ok, let's try.

Working on it. PR tomorrow (Friday) morning.

jasmeet0817 · 2022-04-28T23:32:40Z

Ok, let's try.

Working on it. PR tomorrow (Friday) morning.

Some metrics on latency of the api calls would be nice, so then we can see the impact of your change on the metrics.

monsieurtanuki · 2022-04-29T05:36:41Z

Some metrics on latency of the api calls would be nice, so then we can see the impact of your change on the metrics.

I'm quite optimistic. In addition to that we spare an additional call to the back end.
If it's just a matter of size there are many possible solutions: compress/decompress the json, remove the noise in the json, optimized json. But that's another story.

M123-dev · 2022-04-29T13:46:01Z

Updated the PR @monsieurtanuki @jasmeet0817

monsieurtanuki

Thank you @M123-dev, now it's much easier to read (and to approve), isn't it?

teolemon · 2022-04-29T14:00:13Z

It is easier to merge as well once it's approved, but I will leave @M123-dev the privilege 😊

M123-dev · 2022-04-29T14:11:08Z

Definitely @monsieurtanuki and thanks @teolemon

jasmeet0817

Also looks good to me

M123-dev added 3 commits April 27, 2022 12:57

feat: Make attributes clickable

f5fa365

feat: Make ScoreCard clickable

dffc585

fix: Imports

c50d4e0

M123-dev added attributes 📖 Knowledge panels labels Apr 27, 2022

M123-dev requested review from jasmeet0817 and monsieurtanuki April 27, 2022 11:31

M123-dev requested a review from a team as a code owner April 27, 2022 11:31

github-actions bot added 🥫 Product page summary card labels Apr 27, 2022

M123-dev mentioned this pull request Apr 27, 2022

feat: Make attributes clickable #1654

Closed

jasmeet0817 reviewed Apr 27, 2022

View reviewed changes

packages/smooth_app/lib/pages/product/summary_card.dart Outdated Show resolved Hide resolved

jasmeet0817 reviewed Apr 27, 2022

View reviewed changes

packages/smooth_app/lib/pages/product/summary_card.dart Outdated Show resolved Hide resolved

jasmeet0817 reviewed Apr 27, 2022

View reviewed changes

packages/smooth_app/lib/pages/product/summary_card.dart Outdated Show resolved Hide resolved

jasmeet0817 reviewed Apr 27, 2022

View reviewed changes

teolemon assigned M123-dev Apr 27, 2022

teolemon added this to the V1 milestone Apr 27, 2022

Applied review

e463b65

M123-dev added 2 commits April 27, 2022 19:37

Merge branch 'develop' into make-attributes-clickable-2

1c78af8

Update summary_card.dart

0f95276

github-actions bot added the merge conflicts label Apr 28, 2022

monsieurtanuki mentioned this pull request Apr 28, 2022

feat: knowledgePanels are now loaded with the other product fields #1691

Merged

M123-dev added 3 commits April 29, 2022 15:35

Merge branch 'develop' into make-attributes-clickable-2

326a2f7

simplification after query merge

ec0049e

Format

746a035

github-actions bot removed the merge conflicts label Apr 29, 2022

monsieurtanuki approved these changes Apr 29, 2022

View reviewed changes

teolemon approved these changes Apr 29, 2022

View reviewed changes

M123-dev merged commit bcb3b0e into develop Apr 29, 2022

M123-dev deleted the make-attributes-clickable-2 branch April 29, 2022 14:11

github-actions bot mentioned this pull request Apr 29, 2022

chore(develop): release 1.0.0 #962

Closed

jasmeet0817 reviewed May 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Make attributes clickable (without Provider) #1666

feat: Make attributes clickable (without Provider) #1666

M123-dev commented Apr 27, 2022

codecov-commenter commented Apr 27, 2022

monsieurtanuki commented Apr 27, 2022

jasmeet0817 Apr 27, 2022

monsieurtanuki Apr 27, 2022

jasmeet0817 Apr 27, 2022

M123-dev Apr 27, 2022

jasmeet0817 Apr 27, 2022

M123-dev Apr 27, 2022

M123-dev commented Apr 27, 2022

M123-dev commented Apr 28, 2022

monsieurtanuki commented Apr 28, 2022

jasmeet0817 commented Apr 28, 2022

monsieurtanuki commented Apr 28, 2022

monsieurtanuki commented Apr 28, 2022

teolemon commented Apr 28, 2022

monsieurtanuki commented Apr 28, 2022

jasmeet0817 commented Apr 28, 2022

monsieurtanuki commented Apr 29, 2022

M123-dev commented Apr 29, 2022

monsieurtanuki left a comment

teolemon commented Apr 29, 2022

M123-dev commented Apr 29, 2022

jasmeet0817 left a comment

feat: Make attributes clickable (without Provider) #1666

feat: Make attributes clickable (without Provider) #1666

Conversation

M123-dev commented Apr 27, 2022

What

codecov-commenter commented Apr 27, 2022

Codecov Report

monsieurtanuki commented Apr 27, 2022

jasmeet0817 Apr 27, 2022

Choose a reason for hiding this comment

monsieurtanuki Apr 27, 2022

Choose a reason for hiding this comment

jasmeet0817 Apr 27, 2022

Choose a reason for hiding this comment

M123-dev Apr 27, 2022

Choose a reason for hiding this comment

jasmeet0817 Apr 27, 2022

Choose a reason for hiding this comment

M123-dev Apr 27, 2022

Choose a reason for hiding this comment

M123-dev commented Apr 27, 2022

M123-dev commented Apr 28, 2022

monsieurtanuki commented Apr 28, 2022

jasmeet0817 commented Apr 28, 2022

monsieurtanuki commented Apr 28, 2022

monsieurtanuki commented Apr 28, 2022

teolemon commented Apr 28, 2022

monsieurtanuki commented Apr 28, 2022

jasmeet0817 commented Apr 28, 2022

monsieurtanuki commented Apr 29, 2022

M123-dev commented Apr 29, 2022

monsieurtanuki left a comment

Choose a reason for hiding this comment

teolemon commented Apr 29, 2022

M123-dev commented Apr 29, 2022

jasmeet0817 left a comment

Choose a reason for hiding this comment