Skip to content
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: 4628 - new "reorder Knowledge Panels" feature from dev mode #4778

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

monsieurtanuki
Copy link
Contributor

What

  • There's a new flag in dev mode: "User ordered knowledge panels" (false by default)
  • With this flag on, in the product page the user can control the order of the knowledge panels (new button at the end of the page, new "reorder attributes" page).
  • This order will be kept for all products.
  • That means that if I'm rather interested in Nova and ingredients, I can put them first on each product screen (that is, after the List / Edit / Share buttons).

Screenshot

dev mode: new flag product page: new "reorder" button
Screenshot_1699715418 Screenshot_1699715476
new "reorder" page reordered KP
Screenshot_1699715468 Screenshot_1699715490

Fixes bug(s)

Files

New files:

  • reorderable_knowledge_panel_page.dart: Page where the user can reorder the Knowledge Panel Cards.
  • reordered_knowledge_panel_cards.dart: Knowledge Panel Cards as reordered by the user.
  • standard_knowledge_panel_page.dart: Knowledge Panel Cards as provided by the server.

Impacted files:

  • app_en.arb: added a "reorder the attributes" label
  • knowledge_panel_card.dart: added an isClickable parameter to be get rid of the click icon+effect when reordering attributes
  • knowledge_panel_expanded_card.dart: added an isClickable parameter
  • knowledge_panel_group_card.dart: added an isClickable parameter
  • knowledge_panel_page.dart: explicitly setting isClickable: true
  • knowledge_panels_builder.dart: added an isClickable parameter
  • new_product_page.dart: now using a dev-mode flag to determine if we display the reorder feature; moved code to new class StandardKnowledgePanelCards
  • pubspec.lock: wtf
  • score_card.dart: unrelated minor refactoring
  • user_preferences.dart: added the user ordered attribute list as preference
  • user_preferences_dev_mode.dart: added a flag for user reordered attribute feature

New files:
* `reorderable_knowledge_panel_page.dart`: Page where the user can reorder the Knowledge Panel Cards.
* `reordered_knowledge_panel_cards.dart`: Knowledge Panel Cards as reordered by the user.
* `standard_knowledge_panel_page.dart`: Knowledge Panel Cards as provided by the server.

Impacted files:
* `app_en.arb`: added a "reorder the attributes" label
* `knowledge_panel_card.dart`: added an `isClickable` parameter to be get rid of the click icon+effect when reordering attributes
* `knowledge_panel_expanded_card.dart`: added an `isClickable` parameter
* `knowledge_panel_group_card.dart`: added an `isClickable` parameter
* `knowledge_panel_page.dart`: explicitly setting `isClickable: true`
* `knowledge_panels_builder.dart`: added an `isClickable` parameter
* `new_product_page.dart`: now using a dev-mode flag to determine if we display the reorder feature; moved code to new class `StandardKnowledgePanelCards`
* `pubspec.lock`: wtf
* `score_card.dart`: unrelated minor refactoring
* `user_preferences.dart`: added the user ordered attribute list as preference
* `user_preferences_dev_mode.dart`: added a flag for user reordered attribute feature
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner November 11, 2023 15:24
@github-actions github-actions bot added 🥫 Product page Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🌐 l10n labels Nov 11, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #4778 (396eaf5) into develop (9ee3c93) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           develop   #4778      +/-   ##
==========================================
- Coverage     9.89%   9.85%   -0.05%     
==========================================
  Files          312     315       +3     
  Lines        15809   15878      +69     
==========================================
  Hits          1565    1565              
- Misses       14244   14313      +69     
Files Coverage Δ
...e_panel/knowledge_panels/knowledge_panel_page.dart 0.00% <ø> (ø)
.../lib/knowledge_panel/knowledge_panels_builder.dart 0.00% <ø> (ø)
...nowledge_panels/knowledge_panel_expanded_card.dart 0.00% <0.00%> (ø)
...l/knowledge_panels/knowledge_panel_group_card.dart 0.00% <0.00%> (ø)
...es/smooth_app/lib/cards/data_cards/score_card.dart 0.00% <0.00%> (ø)
...b/pages/preferences/user_preferences_dev_mode.dart 0.00% <0.00%> (ø)
.../lib/data_models/preferences/user_preferences.dart 19.28% <0.00%> (-0.87%) ⬇️
...e_panel/knowledge_panels/knowledge_panel_card.dart 0.00% <0.00%> (ø)
...pages/product/reordered_knowledge_panel_cards.dart 0.00% <0.00%> (ø)
...smooth_app/lib/pages/product/new_product_page.dart 0.00% <0.00%> (ø)
... and 2 more

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link
Member

@teolemon teolemon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get what's going on visually. It looks that everything was flattened (the screen with the reorder button), and I can't visually understand where I am. Also, there seems to be some duplicated data (packaging)

@monsieurtanuki
Copy link
Contributor Author

I don't get what's going on visually. It looks that everything was flattened (the screen with the reorder button) and I can't visually understand where I am.

Of course everything is flattened, as all KP "items" are now considered equal. If we put those items inside "Health" or "Environment" cards, then the concept of reordering cannot be achieved, can it? Imagine I want to put Nova, Ecoscore and Saturated Fats on top, should we say "Health / Nova", "Environment / Ecoscore" then "Health: Saturated Fats"?

Also, there seems to be some duplicated data (packaging)

That's probably because my screenshots were at too early a stage of my dev, when I didn't discriminate between panels and panel_groups.
But anyway there's going to be duplicates: if you set "Saturated fats" as mandatory, you'll see it both on top of the product and wherever you reordered it. It's already the case now, but it looks less silly as you have no control today on the order.

@g123k
Copy link
Collaborator

g123k commented Nov 15, 2023

Generally speaking, it's OK for, as the idea would be to pick the items we want, then we can reorder them.
I just wonder if the code should not be displayed temporary in a new screen, as it will be a different tab in the new design

@monsieurtanuki
Copy link
Contributor Author

I just wonder if the code should not be displayed temporary in a new screen, as it will be a different tab in the new design

@g123k If you're worried about blowtorching the code when you refactor the product page, keep in mind that the new display is in a separate file, reordered_knowledge_panel_cards.dart. Which would already be suitable for a new tab.

@monsieurtanuki
Copy link
Contributor Author

@teolemon You can consider it as a POC, with potential iterative improvements.

@g123k
Copy link
Collaborator

g123k commented Nov 17, 2023

Code-wise, I have a general question about the implementation.
You allow mixing any kind of data (e.g.: health - environment - health…).

I have not a strong opinion on this and this is an opened question, but shouldn't we have a kind of hierarchy to force grouping by section?

@monsieurtanuki
Copy link
Contributor Author

I have not a strong opinion on this and this is an opened question, but shouldn't we have a kind of hierarchy to force grouping by section?

That's the way I interpreted it. Like, I care about packagings and NOVA, and not really about the rest.
We can reorder within each section, and then display both sections (health + eco). Display them each in a separate tab? Even reordering them (health + eco or eco + health)?
I feel like this distinction between health and eco doesn't make much sense if I can reorder the way I want. I'm glad if there's no fat, no saturated fat, no sugar, no allergens, but it's not my priority and I don't want to have to always skip these data to get to what matters to me, especially as I reordered within each section what mattered most.

Or, given that each user (like me) has a limited interest in ALL data, pinning specific data would be the answer. I don't want to reorder all, I'm just interested in a couple of food data, so I pin them, and see them on top of everything.
The historic order of the pinning could even imply the displayed order of the top food data.
Then, the standard health and eco sections, without reordering.

@monsieurtanuki
Copy link
Contributor Author

@g123k @teolemon I can't remember if reordering KPs was part of the POC.

  1. If so, how close is this PR from what was implemented in the POC?
  2. If not, as a user I would prefer to pin specific KPs than to reorder all of them.
  3. If needed, I can reorder health and eco KPs in two distinct pages, but I'm not convinced it's very user friendly

@teolemon teolemon changed the title feat: 4628 - new "reorder KP" feature from dev mode feat: 4628 - new "reorder Knowledge Panels" feature from dev mode Nov 22, 2023
@g123k
Copy link
Collaborator

g123k commented Nov 22, 2023

@g123k @teolemon I can't remember if reordering KPs was part of the POC.

  1. If so, how close is this PR from what was implemented in the POC?
  2. If not, as a user I would prefer to pin specific KPs than to reorder all of them.
  3. If needed, I can reorder health and eco KPs in two distinct pages, but I'm not convinced it's very user friendly

This was tested at the beginning of the POC to know if people were interested, but never designed.
Your implementation is still relevant as this feature is local-only.

The only drawback pointed out by @teolemon is actually, it's step 2 of the issue.
Step 1: you select the items you want / Step 2: you can reorder them

@monsieurtanuki
Copy link
Contributor Author

Step 1: you select the items you want / Step 2: you can reorder them

@g123k @teolemon I cannot picture exactly what you mean.
For the moment we have a Health KP block, then an Eco KP block.

  • how would you trigger the "select items" feature? with a specific button, like the "reorder attributes" in the current PR?
  • would there be a second step where you can reorder the selected attributes?
  • what about the different blocks - keep them apart, join them?

TBH I would appreciate a verbose description of what is supposed to happen here.

Possible scenario: I can somehow remove the actual "reorder items" feature from this PR and keep most of the induced refactoring. That way we would be ready when we have a clear view of what to do, especially on the new tab'ed product page.

@teolemon
Copy link
Member

The closest mockup we have at the moment are this one it is meant to be for the homepage but it gives you an indication of where we could go.

Screenshot_20231128-145201.png

The second mockup gives you a sense of what the 4u page will look like.

Screenshot_20231128-145214.png

cc @g123k

@g123k
Copy link
Collaborator

g123k commented Nov 28, 2023

The idea of the "For me" tab will be split in two parts:

  • The list of all info related to your food prefs
  • An extra space allowing you to select the items you want (eg: I only want the NOVA and the carbon footprint)

That's why for the second part, we have to first ask for the items the user wants, then offer the ability to reorder them

@monsieurtanuki
Copy link
Contributor Author

@g123k @teolemon There are still some minor UI details to fix, but would the following be OK with you?

empty "for me" select attributes
Screenshot_1701187440 Screenshot_1701187453
reorder attributes "for me"
Screenshot_1701187464 Screenshot_1701187471

@g123k
Copy link
Collaborator

g123k commented Dec 4, 2023

Seems better for me, I would just change the title of the first step with something like "Personal feed: select the attributes"

@monsieurtanuki
Copy link
Contributor Author

Seems better for me, I would just change the title of the first step with something like "Personal feed: select the attributes"

@g123k In those cases, please approve the PR first and then I'll do the minor changes. With just a couple of reviewers at the moment, we need to remove frictions in the PR approval process in order to have smoother code integration. Assuming that smoother code integration is a good idea.

@teolemon
Copy link
Member

teolemon commented Dec 8, 2023

It will be DEV mode, right ? If so, we can potentially merge @monsieurtanuki, but then I'll add it to my newly created tracker of unfinished features.
As it stands right now, I see several issues:

  • The tabs in the products page are not here yet, and thus the feature is not integrated at its final place
  • The ergonomy is not quite there yet (we probably need filtering, and something more zen and structured (due to the sheer volume of knowledge panels)

@g123k
Copy link
Collaborator

g123k commented Dec 8, 2023

Seems better for me, I would just change the title of the first step with something like "Personal feed: select the attributes"

@g123k In those cases, please approve the PR first and then I'll do the minor changes. With just a couple of reviewers at the moment, we need to remove frictions in the PR approval process in order to have smoother code integration. Assuming that smoother code integration is a good idea.

My issue with approving partial PR was that we didn't track the missing features.
We've discussed this with @teolemon and there is now #4881.
So I will approve and then we have to add my suggestions to the tracker

@monsieurtanuki monsieurtanuki merged commit 60971d1 into openfoodfacts:develop Dec 8, 2023
6 checks passed
@monsieurtanuki
Copy link
Contributor Author

Thank you @teolemon @g123k for your comments and reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 Knowledge panels 🌐 l10n Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🥫 Product page
Projects
None yet
Development

Successfully merging this pull request may close these issues.

For you: allow selecting personal values from knowledge panels
4 participants