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: #2174 - doomscrolling instead of "download more" button #2770

Merged
merged 2 commits into from
Aug 11, 2022

Conversation

monsieurtanuki
Copy link
Contributor

Impacted file:

  • Podfile.lock: wtf (ios: pod update Sentry)
  • product_query_page.dart

What

  • On product query page, there's no "download more" button anymore
  • It's been replaced by the doomscrolling mode:
    • we display the whole list of products, even if we don't know them (not even their barcodes)
    • we trigger automatically the download of the next page, when the first unknown product is asked by the ListView
    • when we are loading the next page we don't hide the previous results with a dialog or a progress indicator
  • pending questions:
    • maybe we should call the "load next page" method earlier than exactly when we need it, like for first unknown page index - 5?
    • maybe we should display something specific for data being downloaded (the next page) and other unknown data (beyond next page)
  • We will probably have to rethink our queries, because of the possible inconsistencies between pages during a long period. Should we query just ALL the barcodes, and let the list download each product one by one if needed? Btw that's not an issue that appeared specifically with that PR. To be continued.

Screenshot

When you go beyond the known area Which smoothly refreshes into
Capture d’écran 2022-08-10 à 18 37 10 Capture d’écran 2022-08-10 à 18 37 59

Fixes bug(s)

…utton

Impacted file:
* `product_query_page.dart`
  * we display the whole list of products, even if we don't know them (not even their barcodes)
  * we trigger automatically the download of the next page, when the first unknown product is asked by the `ListView`
  * when we are loading the next page we don't hide the previous results with a dialog or a progress indicator
…utton

Impacted file:
* `Podfile.lock`: wtf
* `product_query_page.dart`
  * we display the whole list of products, even if we don't know them (not even their barcodes)
  * we trigger automatically the download of the next page, when the first unknown product is asked by the `ListView`
  * when we are loading the next page we don't hide the previous results with a dialog or a progress indicator
@codecov-commenter
Copy link

Codecov Report

Merging #2770 (27a823d) into develop (2ea0da3) will decrease coverage by 1.72%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2770      +/-   ##
==========================================
- Coverage     8.86%   7.13%   -1.73%     
==========================================
  Files          161     218      +57     
  Lines         6623   10619    +3996     
==========================================
+ Hits           587     758     +171     
- Misses        6036    9861    +3825     
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 62.26% <0.00%> (-20.72%) ⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart 15.11% <0.00%> (-19.10%) ⬇️
...mooth_app/lib/data_models/product_preferences.dart 21.68% <0.00%> (-9.75%) ⬇️
packages/smooth_app/lib/main.dart 14.65% <0.00%> (-3.24%) ⬇️
.../smooth_app/lib/pages/onboarding/welcome_page.dart 0.00% <0.00%> (-3.13%) ⬇️
.../smooth_app/lib/pages/onboarding/scan_example.dart 0.00% <0.00%> (-2.28%) ⬇️
...ackages/smooth_app/lib/pages/scan/scan_header.dart 2.50% <0.00%> (-2.27%) ⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart 0.00% <0.00%> (-1.57%) ⬇️
... and 237 more

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

@teolemon teolemon added ✨ enhancement New feature or request 🔎 Search labels Aug 10, 2022
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.

like for first unknown page index - 5? >> perhaps one or two page ahead, given the speed ?

Comment on lines +226 to 229
if (index >= _model.displayBarcodes.length) {
// TODO(monsieurtanuki): maybe display something specific for data being downloaded (the next page) and unknown data (beyond next page)
return const SmoothProductCardTemplate();
}
Copy link
Member

Choose a reason for hiding this comment

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

One thing, the template has no common loading animation it is just a plan thing, if it takes over a certain amount of time and the user can scroll as long as he want's it could create the illusion that something is broken, therefor I'd suggest to at max show only one template at a time.

Copy link
Member

@teolemon teolemon Aug 11, 2022

Choose a reason for hiding this comment

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

it's usually solved using a shimmering effect that suggests loading

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but I'd still prefer a single one and as we know monsieurtanuki isn't a friend of fancy animations so thats something we can always add later

Copy link
Member

Choose a reason for hiding this comment

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

ok

@teolemon teolemon merged commit c821f6d into openfoodfacts:develop Aug 11, 2022
@monsieurtanuki
Copy link
Contributor Author

@teolemon @M123-dev Oops, I had minor changes about to be pushed. What do you think of that:

const int numberOfPagesBeforeDownload = 1;
const int numberOfDownloadingPages = 1;
// if not all products are already downloaded
if (_model.supplier.partialProductList.totalSize >
    _model.displayBarcodes.length) {
  if (index >=
      _model.displayBarcodes.length -
          numberOfPagesBeforeDownload *
_model.supplier.productQuery.pageSize) {
    _downloadNextPage();
  }
}
if (index >=
    _model.displayBarcodes.length +
        numberOfDownloadingPages *
            _model.supplier.productQuery.pageSize) {
  return const SmoothProductCardTemplate(
    message: 'Will be loaded later',
  );
}
if (index >= _model.displayBarcodes.length) {
  return SmoothProductCardTemplate(
    message: _loadingNext ? 'Loading...' : 'Could not load',
  );
}

@M123-dev
Copy link
Member

Looks great, I'd prefer a animation but a yeah would be good to have.
can maybe later be used for showing a animation / a error template card

@monsieurtanuki
Copy link
Contributor Author

@M123-dev Now the code is merged - without the latest suggested changes.
I guess that while using the app with the doomscrolling, developers will have various suggestions around both topics: download in advance a bit earlier, and how to display data that is not loaded yet.
Feel free to create a new issue / PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request 🔎 Search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the "load more" mechanism more user friendly in product search
4 participants