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: 4041 - renamed "history" bottom item as "lists", w/ access to other lists #4277

Merged
merged 5 commits into from
Jul 12, 2023

Conversation

monsieurtanuki
Copy link
Contributor

What

  • The "history" bottom navigation bar item is now renamed as "lists", with a different icon.
  • From the product list page, we can go the other lists. For the moment, the other lists are among "scan session", "scan history" and "history".
  • In a next PR, user lists will be added.

Screenshots

history page history page with popup menu
Screenshot_2023-07-06-11-35-04 Screenshot_2023-07-06-11-35-11
scan history page scan session page
Screenshot_2023-07-06-11-35-23 Screenshot_2023-07-06-11-35-16

Part of

Files

New file:

  • product_list_popup_items.dart: Popup menu items for the product list page.

Impacted files:

  • app_en.arb: added 2 labels
  • page_manager.dart: new label and icon for bottom bar
  • product_list_page.dart: added fast access to scan session, scan history and history; moved code to product_list_popup_items.dart
  • product_query_page_helper.dart: minor refactoring
  • scan_header.dart: minor refactoring
  • tab_navigator.dart: minor refactoring
  • user_preferences_account.dart: minor refactoring

…ss to other lists

New file:
* `product_list_popup_items.dart`: Popup menu items for the product list page.

Impacted files:
* `app_en.arb`: added 2 labels
* `page_manager.dart`: new label and icon for bottom bar
* `product_list_page.dart`: added fast access to scan session, scan history and history; moved code to `product_list_popup_items.dart`
* `product_query_page_helper.dart`: minor refactoring
* `scan_header.dart`: minor refactoring
* `tab_navigator.dart`: minor refactoring
* `user_preferences_account.dart`: minor refactoring
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner July 6, 2023 09:52
@github-actions github-actions bot added 🥫 Product page 🌐 l10n 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… User lists labels Jul 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2023

Codecov Report

Merging #4277 (7150c12) into develop (a5fb0e1) will decrease coverage by 0.02%.
The diff coverage is 2.46%.

@@             Coverage Diff             @@
##           develop    #4277      +/-   ##
===========================================
- Coverage    10.82%   10.80%   -0.02%     
===========================================
  Files          286      287       +1     
  Lines        14247    14273      +26     
===========================================
  Hits          1542     1542              
- Misses       12705    12731      +26     
Impacted Files Coverage Δ
packages/smooth_app/lib/pages/page_manager.dart 0.00% <0.00%> (ø)
...pp/lib/pages/product/common/product_list_page.dart 0.00% <0.00%> (ø)
...pages/product/common/product_list_popup_items.dart 0.00% <0.00%> (ø)
...ages/product/common/product_query_page_helper.dart 27.08% <ø> (-1.49%) ⬇️
...ackages/smooth_app/lib/pages/scan/scan_header.dart 0.00% <ø> (ø)
packages/smooth_app/lib/widgets/tab_navigator.dart 0.00% <0.00%> (ø)
...ib/pages/preferences/user_preferences_account.dart 62.29% <100.00%> (+0.20%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@g123k
Copy link
Collaborator

g123k commented Jul 12, 2023

Hi @monsieurtanuki!

Thanks a lot for your PR. Generally speaking in terms of code, that looks OK.

But after trying your implementation on a device, I'm not convinced about the UX.
Having a Lists item in the bottom bar is OK, but it's confusing that when clicking on it, we have a History App Bar label.
A simple fix could be, to change the title to: Lists: My history or something like that.

The menu is also confusing because we mix external actions (eg: Share / View on the web) and direct actions on the list.

Here are my suggestions:

  • Keep Share and View on the web in the hidden menu
  • Create a dedicated icon in the AppBar to switch between lists (there is no icon for that, but I can create a custom one, which would like this one, but with a list)
  • Create a dedicated Clear your history button (with the delete_sweep icon).

What do you think of this change?

@monsieurtanuki
Copy link
Contributor Author

A simple fix could be, to change the title to: Lists: My history or something like that.

I would put the most important thing first, like History (list), but I get the idea.

The menu is also confusing because we mix external actions (eg: Share / View on the web) and direct actions on the list.

That's the general idea of a menu, and that's why they usually put separators.
Capture d’écran 2023-07-12 à 09 38 10

Keep Share and View on the web in the hidden menu

Understood.

Create a dedicated icon in the AppBar to switch between lists (there is no icon for that, but I can create a custom one, which would like this one, but with a list)

I don't believe in custom icons.

Create a dedicated Clear your history button (with the delete_sweep icon).

I don't want to overload the app bar with buttons (I have a small screen, remember). Btw the label is incorrect: we clear whatever list (not just history).

To summarize:

  • we can change the title of the app bar in order to introduce explicitly the label "list"
  • we may split in two the menu actions
    • one "more" button with the actions on that list (rename, clear, share, view in web, ...)
    • one "other list" button

@g123k What do you think of those improvements?

@g123k
Copy link
Collaborator

g123k commented Jul 12, 2023

After thinking a little bit, here is what it could look like, with a separation between generic actions and a switch between lists:

We change the title as suggested + we have a dedicated icon to switch between lists (the icon is incorrect here)
1

The menu is now simplified (icons are not mandatory, it was just a test):
2

And the item to switch between lists also allows creating a new one and having the custom ones also.
3

This would allow removing the entry from the Profile.

EDIT: The menu containing lists could be a bottom sheet also
BottomSheet

@monsieurtanuki
Copy link
Contributor Author

full page "..." menu list icon
Screenshot_2023-07-12-16-28-23 Screenshot_2023-07-12-16-28-34 Screenshot_2023-07-12-16-28-44

Using a standard cupertino icon and the standard cupertino bottom sheet.

@g123k
Copy link
Collaborator

g123k commented Jul 12, 2023

full page "..." menu list icon
Screenshot_2023-07-12-16-28-23 Screenshot_2023-07-12-16-28-34 Screenshot_2023-07-12-16-28-44
Using a standard cupertino icon and the standard cupertino bottom sheet.

I love the change.
But why do you use a Cupertino Widget for this?
As part of #4281, there is now a SmoothModalSheet, which could be perfect here:

252021271-a0a589a3-76c6-490f-9a8d-6bffc5138570

@monsieurtanuki
Copy link
Contributor Author

@g123k I've just pushed a code that should be OK for you.
I'm really not convinced by the additional " (liste)" label, which looks very redundant with the navigation tab label and with the app bar icon.
Please confirm that you prefer it and I'll localize it.

full page "..." menu list icon
Screenshot_2023-07-12-18-22-18 Screenshot_2023-07-12-18-22-24 Screenshot_2023-07-12-18-22-29

@g123k
Copy link
Collaborator

g123k commented Jul 12, 2023

@g123k I've just pushed a code that should be OK for you. I'm really not convinced by the additional " (liste)" label, which looks very redundant with the navigation tab label and with the app bar icon. Please confirm that you prefer it and I'll localize it.

full page "..." menu list icon
Screenshot_2023-07-12-18-22-18 Screenshot_2023-07-12-18-22-24 Screenshot_2023-07-12-18-22-29

Yes indeed, you're right, it's a bit awkward. I will think about a better solution.
For the Modal Sheet, I will probably test of few UI, as I'm not really fan of buttons, but keep it like so, it's a good start 👍

@monsieurtanuki monsieurtanuki requested a review from g123k July 12, 2023 16:48
@monsieurtanuki
Copy link
Contributor Author

I've just removed the " (liste)"label.

@g123k g123k merged commit cac58a2 into openfoodfacts:develop Jul 12, 2023
@g123k
Copy link
Collaborator

g123k commented Jul 12, 2023

Thanks a lot for your work @monsieurtanuki

@monsieurtanuki
Copy link
Contributor Author

Thank you @g123k for your review!
The next part (with user lists) is less trivial, as we need a refreshed list of user lists, which is refreshed async.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌐 l10n 🥫 Product page 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… User lists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants