Skip to content

feat: Add infinite scrolling to the various Open Prices ListViews. #6561

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 26 commits into
base: develop
Choose a base branch
from
Open

feat: Add infinite scrolling to the various Open Prices ListViews. #6561

wants to merge 26 commits into from

Conversation

AshutoshKhadse23
Copy link
Contributor

Closes: #6478

I added infinite scrolling to open the prices list view.

This is how the solution looks :

issue.6478.new.mp4

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 365 lines in your changes missing coverage. Please review.

Project coverage is 5.79%. Comparing base (4d9c7fc) to head (00fd768).
Report is 862 commits behind head on develop.

Files with missing lines Patch % Lines
...mooth_app/lib/pages/prices/prices_proofs_page.dart 0.00% 104 Missing ⚠️
...oth_app/lib/pages/prices/infinite_scroll_list.dart 0.00% 69 Missing ⚠️
...th_app/lib/pages/prices/prices_locations_page.dart 0.00% 49 Missing ⚠️
..._app/lib/pages/prices/infinite_scroll_manager.dart 0.00% 46 Missing ⚠️
...smooth_app/lib/pages/prices/prices_users_page.dart 0.00% 39 Missing ⚠️
...ooth_app/lib/pages/prices/product_prices_list.dart 0.00% 30 Missing ⚠️
...oth_app/lib/pages/prices/prices_products_page.dart 0.00% 28 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           develop   #6561      +/-   ##
==========================================
- Coverage     9.54%   5.79%   -3.76%     
==========================================
  Files          325     503     +178     
  Lines        16411   30127   +13716     
==========================================
+ Hits          1567    1745     +178     
- Misses       14844   28382   +13538

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@monsieurtanuki monsieurtanuki self-requested a review April 22, 2025 10:12
monsieurtanuki
monsieurtanuki reviewed Apr 22, 2025
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @AshutoshKhadse23!

I have no strong opinion against your code, that seems to work. Possibly minor comments here and there.

The thing is that for the Prices (in a broad sense) we need different objects with a "load more" feature: prices (with this PR), proofs, contributors, locations and products.

Do you feel confident enough in your OOP skills? If so you should refactor your current code with generic concepts like:

  • a loading class
    • with a load method that has the page number as a parameter
    • returning a list of items, a number of items, a number of pages
  • an item display class that takes an item as a parameter and displays it
  • a "load by page" class that displays items by page, with a "loader" and an "item display" in the constructor

I'm sure there are Design Patterns around those common features.

What do you think about that?

@AshutoshKhadse23
Copy link
Contributor Author

@monsieurtanuki, I tried to implement what you suggested—looking forward to your feedback!

monsieurtanuki
monsieurtanuki reviewed Apr 24, 2025
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @AshutoshKhadse23!
Good job!
Please have a look at my comments, though, for code simplification.

@AshutoshKhadse23
Copy link
Contributor Author

Hello @monsieurtanuki ,
Updated code according to the comments. And is ready for review.

monsieurtanuki
monsieurtanuki reviewed Apr 25, 2025
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @AshutoshKhadse23!
Obviously something wrong in your latest push, as we have twice the same class.
The idea was to get rid of parameters that are ALWAYS the same, and that's not was you pushed.

@AshutoshKhadse23
Copy link
Contributor Author

@monsieurtanuki, changed the code according to the comment.

monsieurtanuki
monsieurtanuki reviewed Apr 26, 2025
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @AshutoshKhadse23!
Please have a look at my comments.

@AshutoshKhadse23
Copy link
Contributor Author

@monsieurtanuki, changed the code according to the comment.

monsieurtanuki
monsieurtanuki reviewed Apr 27, 2025
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @AshutoshKhadse23
Please have a look at my comments.

@AshutoshKhadse23
Copy link
Contributor Author

@monsieurtanuki, changed the code according to the comment.

monsieurtanuki
monsieurtanuki reviewed May 3, 2025
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @AshutoshKhadse23!
Very interesting PR, isn't it?
I found a lot of inspiration reading your code. For the better I hope.

@AshutoshKhadse23
Copy link
Contributor Author

Hi @AshutoshKhadse23! Very interesting PR, isn't it? I found a lot of inspiration reading your code. For the better I hope.

@monsieurtanuki, I’m also finding this PR really interesting! It’s been a great way to learn more about the codebase.

@AshutoshKhadse23
Copy link
Contributor Author

@monsieurtanuki,
You are right !!
So changed the code according to the concept of OPP Java

monsieurtanuki
monsieurtanuki reviewed May 4, 2025
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @AshutoshKhadse23!
I cannot comment everything, but on your way to OOP you experienced some confusion (like casually returning empty SizedBox()) and "just-in-case"ism (like a refresh method that is NEVER used).

packages/smooth_app/lib/pages/prices/infinite_scroll_manager.dart
int? get totalPages => _totalPages;

/// Abstract method to implement the data fetching logic for a specific page
/// This method must be implemented by subclasses to handle API calls or data fetching
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove that comment.
Of course it needs to be implemented.

packages/smooth_app/lib/pages/prices/prices_products_page.dart
Comment on lines +86 to +87
required this.orderBy,
required this.pageSize,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point? They always have the same values, don't they?

packages/smooth_app/lib/pages/prices/prices_proofs_page.dart
_proofManager = InfiniteScrollProofManager(
initialItems: const <Proof>[],
bearerTokenCallback: () => _bearerToken,
onAuthenticateNeeded: _authenticate,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure it's the page's job to deal with the token, but let's focus on the easier classes first.

packages/smooth_app/lib/pages/prices/prices_proofs_page.dart
) {
return Column(
children: <Widget>[
Builder(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get the point of the Builder layer.
Please comment it or get rid of it.

packages/smooth_app/lib/pages/prices/prices_proofs_page.dart
required Proof item,
required int index,
}) {
return const SizedBox();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nonsense.

packages/smooth_app/lib/pages/prices/product_prices_list.dart
Comment on lines +49 to +50
itemBuilder: (BuildContext context, Price price) =>
_buildPriceItem(context, price),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
itemBuilder: (BuildContext context, Price price) =>
_buildPriceItem(context, price),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monsieurtanuki monsieurtanuki monsieurtanuki left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@AshutoshKhadse23 AshutoshKhadse23

Labels
🌐 l10n Prices
Projects
💸 Open Prices
Status: Backlog
🤳🥫 The Open Food Facts mobile app (Android & iOS)
Status: 💬 To discuss and validate
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add infinite scrolling to the various Open Prices ListViews.
3 participants
@AshutoshKhadse23 @codecov-commenter @monsieurtanuki