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 - now displaying user lists in "List" nav tab #4298

Merged
merged 5 commits into from
Jul 13, 2023

Conversation

monsieurtanuki
Copy link
Contributor

What

  • When the user clicked on the "select the other product lists" button, the user lists were not present.
  • Now the user lists are present, and the user can create new user lists.

Screenshot

Screenshot_2023-07-13-08-23-08

Part of

Files

New file:

  • all_product_list_page.dart: Page that lists all product lists.

Impacted files:

  • all_user_product_list_page.dart: simplified thanks to a new not async getter of all user product lists
  • dao_product_list.dart: split the "get user lists" method in two - one not async, one async with barcode checks
  • new_product_page.dart: minor refactoring
  • product_list_page.dart: now using new page AllProductListPage in order to list all product lists
  • product_list_popup_items.dart: fixed a label; removed a class we don't use anymore as we put lists and actions in different buttons
  • product_list_user_dialog_helper.dart: minor refactoring

New file:
* `all_product_list_page.dart`: Page that lists all product lists.

Impacted files:
* `all_user_product_list_page.dart`: simplified thanks to a new not `async` getter of all user product lists
* `dao_product_list.dart`: split the "get user lists" method in two - one not `async`, one `async` with barcode checks
* `new_product_page.dart`: minor refactoring
* `product_list_page.dart`: now using new page `AllProductListPage` in order to list all product lists
* `product_list_popup_items.dart`: fixed a label; removed a class we don't use anymore as we put lists and actions in different buttons
* `product_list_user_dialog_helper.dart`: minor refactoring
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner July 13, 2023 06:47
@monsieurtanuki monsieurtanuki requested a review from g123k July 13, 2023 06:48
@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. database User lists labels Jul 13, 2023
@monsieurtanuki
Copy link
Contributor Author

Hi @g123k!
I added the feature ("user lists") with a simple interface. I know you have more ambitious UI expectations, but at least the feature is implemented.

}
final AppLocalizations appLocalizations = AppLocalizations.of(context);
return SmoothScaffold(
appBar: SmoothAppBar(title: Text('Select a list')),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot to translate this one

@g123k
Copy link
Collaborator

g123k commented Jul 13, 2023

It seems you also have an issue with a missing const constructor.

Impacted files:
* `all_product_list_page.dart`: now `Stateless`; added a localized label
* `app_en.arb`: added a label for the "select a list" title.
* `new_product_page.dart`: minor fix after merge conflict
@monsieurtanuki
Copy link
Contributor Author

Thank you @g123k for your review!

  • indeed a StatelessWidget is enough - I copied a code that used setState once but in a part that I did not copy
  • I've localized the label - after you implicitly agreed that the UI was decent enough
  • I don't understand itemExtent so I don't use it, and I always chose code maintainability over hypothetical minor performance improvement

@g123k
Copy link
Collaborator

g123k commented Jul 13, 2023

Thank you @g123k for your review!

  • indeed a StatelessWidget is enough - I copied a code that used setState once but in a part that I did not copy
  • I've localized the label - after you implicitly agreed that the UI was decent enough
  • I don't understand itemExtent so I don't use it, and I always chose code maintainability over hypothetical minor performance improvement

Perfect!
For ListView.builder, the idea is that Flutter tries to guess the number of items to draw on the screen.
By giving an itemExtent, we explicitly say that an item has the following height.
In our case, since all items have the same height, giving this info will help to reduce the number of items in cache.

For that, just try a print() with and without it and you will see a difference, which can sometimes be +/-10 cells.

For the UI, what I propose is to validate your implementation with the screen and I will do some tests to potentially include it in a bottom sheet, but later and in another PR. The idea is not to block this PR.

@monsieurtanuki
Copy link
Contributor Author

@g123k For the record I will not add itemExtent. I understand the purpose and I have a personal interest in performances, but in general and specifically in that case (user lists) it will add confusion in the code and potentially problem with accessibility, for a non detectable performance improvement.
Especially if you plan to implement another UI later.
Feel free to approve now so that the PR can be merged.

Copy link
Collaborator

@g123k g123k left a comment

Choose a reason for hiding this comment

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

Ok, I let the code as is.
And I work in parallel, to integrate it in a modal sheet.

Thanks for your work @monsieurtanuki!

@g123k g123k merged commit aa73434 into openfoodfacts:develop Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database 🌐 l10n Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🥫 Product page User lists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants