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: #2244 - new "add categories" button + major product provider refactoring #2276

Merged
merged 1 commit into from
Jun 19, 2022

Conversation

monsieurtanuki
Copy link
Contributor

New file:

  • up_to_date_product_provider.dart: Provider that reflects all the user changes on [Product]s.

Impacted files:

  • add_basic_details_page.dart: refactoring
  • category_picker_page.dart: refactoring
  • edit_ingredients_page.dart: refactoring around UpToDateProductProvider
  • edit_product_page.dart: refactoring around UpToDateProductProvider
  • knowledge_panel_page_template.dart: refactored for code clarity
  • knowledge_panels_builder.dart: refactored for code clarity
  • main.dart: added UpToDateProductProvider in the provider list
  • new_product_page.dart: refactoring around UpToDateProductProvider; removed deprecated code on "additional button"
  • Podfile.lock: wtf
  • product_list_page.dart: refactored around UpToDateProductProvider
  • product_refresher.dart: refactored around UpToDateProductProvider
  • simple_input_page.dart: refactored
  • summary_card.dart: implemented "add categories" button; refactored around UpToDateProductProvider

What

  • This PR implements the "add categories" button on product page.
  • But the main feature is the use of a new provider around product updates: UpToDateProductProvider
  • When we want to know somewhere if a product has changed due to user edits, no need to cumbersome refresh callback methods, just Consume(cf. code).
  • When we edit a product, just call ProductRefresher, that will notify all the consumer.
  • Possible improvements around list refreshes.

Fixes bug(s)

…t provider refactoring

New file:
* `up_to_date_product_provider.dart`: Provider that reflects all the user changes on [Product]s.

Impacted files:
* `add_basic_details_page.dart`: refactoring
* `category_picker_page.dart`: refactoring
* `edit_ingredients_page.dart`: refactoring around `UpToDateProductProvider`
* `edit_product_page.dart`: refactoring around `UpToDateProductProvider`
* `knowledge_panel_page_template.dart`: refactored for code clarity
* `knowledge_panels_builder.dart`: refactored for code clarity
* `main.dart`: added `UpToDateProductProvider` in the provider list
* `new_product_page.dart`: refactoring around `UpToDateProductProvider`; removed deprecated code on "additional button"
* `Podfile.lock`: wtf
* `product_list_page.dart`: refactored around `UpToDateProductProvider`
* `product_refresher.dart`: refactored around `UpToDateProductProvider`
* `simple_input_page.dart`: refactored
* `summary_card.dart`: implemented "add categories" button; refactored around `UpToDateProductProvider`
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner June 14, 2022 15:36
@codecov-commenter
Copy link

Codecov Report

Merging #2276 (db29b7b) into develop (2ea0da3) will decrease coverage by 1.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2276      +/-   ##
==========================================
- Coverage     8.86%   7.79%   -1.07%     
==========================================
  Files          161     180      +19     
  Lines         6623    9118    +2495     
==========================================
+ Hits           587     711     +124     
- Misses        6036    8407    +2371     
Impacted Files Coverage Δ
...kages/smooth_app/lib/widgets/attribute_button.dart 0.00% <0.00%> (-92.00%) ⬇️
...s/smooth_app/lib/data_models/user_preferences.dart 8.73% <0.00%> (-23.57%) ⬇️
packages/smooth_app/lib/themes/smooth_theme.dart 60.00% <0.00%> (-22.98%) ⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart 19.23% <0.00%> (-14.98%) ⬇️
...mooth_app/lib/data_models/product_preferences.dart 26.47% <0.00%> (-4.96%) ⬇️
.../smooth_app/lib/pages/onboarding/welcome_page.dart 0.00% <0.00%> (-3.13%) ⬇️
packages/smooth_app/lib/main.dart 15.31% <0.00%> (-2.58%) ⬇️
.../smooth_app/lib/pages/onboarding/scan_example.dart 0.00% <0.00%> (-2.28%) ⬇️
...s/smooth_app/lib/pages/onboarding/next_button.dart 2.22% <0.00%> (-1.78%) ⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart 0.00% <0.00%> (-1.57%) ⬇️
... and 171 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 77f3d2a...db29b7b. Read the comment docs.

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for this refactoring @monsieurtanuki

Comment on lines +205 to +207
_,
final UpToDateProductProvider provider,
__,
Copy link
Member

Choose a reason for hiding this comment

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

I'm sure this is wanted probably because we don't need the other arguements. Looks odd non the less

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@M123-dev Yeah I know, the idea behind this kind of syntax in dart is to avoid to add a pointless and confusing nth version of BuildContext. Not a big fan of that syntax either.

Comment on lines +756 to +758
if (!await ProductRefresher().checkIfLoggedIn(context)) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR but we should decide for clear rules who can edit what and probably show something like:
"Please login to edit this"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@M123-dev Oh we do display a dialog if the user is not logged in.

@@ -206,7 +199,7 @@ class _EditIngredientsPageState extends State<EditIngredientsPage> {
));
}

return Scaffold(
final Scaffold scaffold = Scaffold(
Copy link
Member

Choose a reason for hiding this comment

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

We started doing this a lot lately, I'm not a fan of having the scroll a lot to find the whole widget tree, what about putting it directly in the Consumer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@M123-dev I agree with you: the reason behind that 2 step return is to avoid that git considers that all the text has changed just because the formatting added 2 spaces when we added a level with an extra child.
There were enough changes in this PR and I didn't want to add work to the code reviewers ;)

@M123-dev
Copy link
Member

Sounds good @monsieurtanuki, feel free to merge

@monsieurtanuki monsieurtanuki merged commit 80d4b9d into openfoodfacts:develop Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Link the on-page add category button now that the dedicated section is working
3 participants