search

firebase / FirebaseUI-Flutter

Apache License 2.0
93 stars 81 forks source link

🐛 [firebase_ui_firestore] Enable showing a progress indicator when fetching more pages #130

Open rorystephenson opened 9 months ago

rorystephenson commented 9 months ago

Is there an existing issue for this?

What plugin is this bug for?

Firebase UI Firestore

What platform(s) does this bug affect?

Android, iOS, Web, macOS, Linux, Windows

List of dependencies used.

flutter pub deps -s list 
Dart SDK 3.1.3
Flutter SDK 3.13.6
firebase_ui_firestore 1.5.11

dependencies:
  - cloud_firestore 4.9.2
  - cloud_firestore_platform_interface ^5.16.1
  - cloud_firestore_web ^3.7.1
  - collection ^1.0.0
  - firebase_core ^2.16.0
  - firebase_core_platform_interface ^4.8.0
  - flutter any
  - meta ^1.8.0
- firebase_ui_localizations 1.6.1
  - flutter any
  - flutter_localizations any
  - path ^1.8.2
- flutter 0.0.0
  - characters 1.3.0
  - collection 1.17.2
  - material_color_utilities 0.5.0
  - meta 1.9.1
  - vector_math 2.1.4
  - web 0.1.4-beta
  - sky_engine any

dev dependencies:
- flutter_test 0.0.0
  - flutter any
  - test_api 0.6.0
  - matcher 0.12.16
  - path 1.8.3
  - fake_async 1.3.1
  - clock 1.1.1
  - stack_trace 1.11.0
  - vector_math 2.1.4
  - async 2.11.0
  - boolean_selector 2.1.1
  - characters 1.3.0
  - collection 1.17.2
  - material_color_utilities 0.5.0
  - meta 1.9.1
  - source_span 1.10.0
  - stream_channel 2.1.1
  - string_scanner 1.2.0
  - term_glyph 1.2.1
  - web 0.1.4-beta
- flutter_lints 2.0.3
  - lints ^2.0.0
- mockito 5.4.2
  - analyzer >=5.11.0 <6.0.0
  - build >=1.3.0 <3.0.0
  - code_builder ^4.5.0
  - collection ^1.15.0
  - dart_style >=1.3.6 <3.0.0
  - matcher ^0.12.15
  - meta ^1.3.0
  - path ^1.8.0
  - source_gen >=0.9.6 <2.0.0
  - test_api >=0.2.1 <0.7.0

transitive dependencies:
- _fe_analyzer_shared 61.0.0
  - meta ^1.0.2
- _flutterfire_internals 1.3.6
  - collection ^1.0.0
  - firebase_core ^2.16.0
  - firebase_core_platform_interface ^4.8.0
  - flutter any
  - meta ^1.8.0
- analyzer 5.13.0
  - _fe_analyzer_shared ^61.0.0
  - collection ^1.17.0
  - convert ^3.0.0
  - crypto ^3.0.0
  - glob ^2.0.0
  - meta ^1.7.0
  - package_config ^2.0.0
  - path ^1.8.0
  - pub_semver ^2.0.0
  - source_span ^1.8.0
  - watcher ^1.0.0
  - yaml ^3.0.0
- args 2.4.2
- async 2.11.0
  - collection ^1.15.0
  - meta ^1.1.7
- boolean_selector 2.1.1
  - source_span ^1.8.0
  - string_scanner ^1.1.0
- build 2.4.1
  - analyzer >=1.5.0 <7.0.0
  - async ^2.5.0
  - convert ^3.0.0
  - crypto ^3.0.0
  - glob ^2.0.0
  - logging ^1.0.0
  - meta ^1.3.0
  - package_config ^2.1.0
  - path ^1.8.0
- built_collection 5.1.1
- built_value 8.6.3
  - built_collection ^5.0.0
  - collection ^1.15.0
  - fixnum ^1.0.0
  - meta ^1.3.0
- characters 1.3.0
- clock 1.1.1
- cloud_firestore_platform_interface 5.16.1
  - _flutterfire_internals ^1.3.6
  - collection ^1.15.0
  - firebase_core ^2.16.0
  - flutter any
  - meta ^1.8.0
  - plugin_platform_interface ^2.1.3
- cloud_firestore_web 3.7.1
  - _flutterfire_internals ^1.3.6
  - cloud_firestore_platform_interface ^5.16.1
  - collection ^1.0.0
  - firebase_core ^2.16.0
  - firebase_core_web ^2.8.0
  - flutter any
  - flutter_web_plugins any
  - js ^0.6.3
- code_builder 4.7.0
  - built_collection ^5.0.0
  - built_value ^8.0.0
  - collection ^1.15.0
  - matcher ^0.12.10
  - meta ^1.3.0
- collection 1.17.2
- convert 3.1.1
  - typed_data ^1.3.0
- crypto 3.0.3
  - typed_data ^1.3.0
- dart_style 2.3.2
  - analyzer >=5.12.0 <7.0.0
  - args >=1.0.0 <3.0.0
  - path ^1.0.0
  - pub_semver >=1.4.4 <3.0.0
  - source_span ^1.4.0
- fake_async 1.3.1
  - clock ^1.1.0
  - collection ^1.15.0
- file 7.0.0
  - meta ^1.9.1
  - path ^1.8.3
- firebase_core 2.16.0
  - firebase_core_platform_interface ^4.8.0
  - firebase_core_web ^2.8.0
  - flutter any
  - meta ^1.8.0
- firebase_core_platform_interface 4.8.0
  - collection ^1.0.0
  - flutter any
  - flutter_test any
  - meta ^1.8.0
  - plugin_platform_interface ^2.1.3
- firebase_core_web 2.8.0
  - firebase_core_platform_interface ^4.8.0
  - flutter any
  - flutter_web_plugins any
  - js ^0.6.3
  - meta ^1.8.0
- fixnum 1.1.0
- flutter_localizations 0.0.0
  - flutter any
  - intl 0.18.1
  - characters 1.3.0
  - clock 1.1.1
  - collection 1.17.2
  - material_color_utilities 0.5.0
  - meta 1.9.1
  - path 1.8.3
  - vector_math 2.1.4
  - web 0.1.4-beta
- flutter_web_plugins 0.0.0
  - flutter any
  - characters 1.3.0
  - collection 1.17.2
  - material_color_utilities 0.5.0
  - meta 1.9.1
  - vector_math 2.1.4
  - web 0.1.4-beta
- glob 2.1.2
  - async ^2.5.0
  - collection ^1.15.0
  - file >=6.1.3 <8.0.0
  - path ^1.8.0
  - string_scanner ^1.1.0
- intl 0.18.1
  - clock ^1.1.0
  - meta ^1.0.2
  - path ^1.8.0
- js 0.6.7
  - meta ^1.7.0
- lints 2.1.1
- logging 1.2.0
- matcher 0.12.16
  - async ^2.10.0
  - meta ^1.8.0
  - stack_trace ^1.10.0
  - term_glyph ^1.2.0
  - test_api >=0.5.0 <0.7.0
- material_color_utilities 0.5.0
  - collection ^1.15.0
- meta 1.9.1
- package_config 2.1.0
  - path ^1.8.0
- path 1.8.3
- plugin_platform_interface 2.1.6
  - meta ^1.3.0
- pub_semver 2.1.4
  - collection ^1.15.0
  - meta ^1.3.0
- sky_engine 0.0.99
- source_gen 1.4.0
  - analyzer >=5.2.0 <7.0.0
  - async ^2.5.0
  - build ^2.1.0
  - dart_style ^2.0.0
  - glob ^2.0.0
  - path ^1.8.0
  - source_span ^1.8.0
  - yaml ^3.0.0
- source_span 1.10.0
  - collection ^1.15.0
  - path ^1.8.0
  - term_glyph ^1.2.0
- stack_trace 1.11.0
  - path ^1.8.0
- stream_channel 2.1.1
  - async ^2.5.0
- string_scanner 1.2.0
  - source_span ^1.8.0
- term_glyph 1.2.1
- test_api 0.6.0
  - async ^2.5.0
  - boolean_selector ^2.1.0
  - collection ^1.15.0
  - meta ^1.3.0
  - source_span ^1.8.0
  - stack_trace ^1.10.0
  - stream_channel ^2.1.0
  - string_scanner ^1.1.0
  - term_glyph ^1.2.0
- typed_data 1.3.2
  - collection ^1.15.0
- vector_math 2.1.4
- watcher 1.1.0
  - async ^2.5.0
  - path ^1.8.0
- web 0.1.4-beta
- yaml 3.1.2
  - collection ^1.15.0
  - source_span ^1.8.0
  - string_scanner ^1.1.0

Steps to reproduce

  1. Run the example app with two minor changes: set pageSize to a small number like 2 and add a loadingBuilder with a debugPrint statement and visible child like a red Container with 100 height.
  2. Wait for the initial load which populates the visible elements.
  3. Scroll to the bottom fast.

Example changes:

diff --git a/packages/firebase_ui_firestore/example/lib/main.dart b/packages/firebase_ui_firestore/example/lib/main.dart
index 50fe4ed..4bf44e8 100644
--- a/packages/firebase_ui_firestore/example/lib/main.dart
+++ b/packages/firebase_ui_firestore/example/lib/main.dart
@@ -34,7 +34,15 @@ class FirebaseUIFirestoreExample extends StatelessWidget {
         appBar: AppBar(title: const Text('Contacts')),
         body: FirestoreListView<User>(
           query: collection,
+          pageSize: 2,
           padding: const EdgeInsets.all(8.0),
+          loadingBuilder: (context) {
+            debugPrint('SHOWING LOADING INDICATOR');
+            return Container(
+              height: 100,
+              color: Colors.red,
+            );
+          },
           itemBuilder: (context, snapshot) {
             final user = snapshot.data();
             return Column(

Expected Behavior

  1. During the initial load the load indicator whilst the visible elements are loading. ✅
  2. When scrolling to the bottom fast, the loading indicator should be visible at the end of the list whilst we are fetching more documents ❌ .

Actual Behavior

During the initial load the loading indicator shows correctly, when fetching subsequent pages it is never shown.

Additional Information

No response

danagbemava-nc commented 9 months ago

I can reproduce the issue using the plugin example code. It seems like this is working as designed (at least from the source code). The current implementation only accounts for snapshot.isFetching (which seems to be for the initial loading) but not for isFetchingMore which would cater to showing a loading indicator when more data is being fetched.

Perhaps we can treat this as a feature request. Labeling for further insight from the team.

cc @lesnitsky

rorystephenson commented 9 months ago

I can reproduce the issue using the plugin example code. It seems like this is working as designed (at least from the source code). The current implementation only accounts for snapshot.isFetching (which seems to be for the initial loading) but not for isFetchingMore which would cater to showing a loading indicator when more data is being fetched.

Perhaps we can treat this as a feature request. Labeling for further insight from the team.

cc @lesnitsky

That makes sense from a code-correctness point of view. From a UX point of view I think it is a bit confusing, when you hit the bottom it stops scrolling and unless you know that more data is being loaded you may not try to drag again once new data has loaded.

Aside from that I was not able to achieve a loading indicator effect when fetching more pages using FirestoreQueryBuilder, isFetchingMore never seemed to be true.

rrousselGit commented 9 months ago

@rorystephenson You need to manually trigger the "fetch next page"

The loading handling of FirestoreListView is limited. There are numerous competing UX standards here, and it cannot really handle them all. The recommended solution is that when you have a case not handled by the default list view, you should switch to the query builder variant and do it yourself.

So there's no bug from a specs point of view. It's more about a feature request.

rorystephenson commented 9 months ago

@rorystephenson You need to manually trigger the "fetch next page"

The loading handling of FirestoreListView is limited. There are numerous competing UX standards here, and it cannot really handle them all. The recommended solution is that when you have a case not handled by the default list view, you should switch to the query builder variant and do it yourself.

So there's no bug from a specs point of view. It's more about a feature request.

@rrousselGit Yep trying to support all the possible UX standards would be a mess. I am already using FirestoreQueryBuilder in my own project because when I used the plain FirestoreListView it triggered too much loading at the start (that's off-topic and there is another issue which seems to be tackling this). I had tried adding a loading indicator but it wasn't showing up, now that I've revisited it I realise that it's not enough to depend on isFetchingMore alone, you need to also show the loading indicator when hasMore is true. I hacked together a working version based on the example app: https://github.com/rorystephenson/FirebaseUI-Flutter/commit/e9fd0ec2fd39b8a7a9f34468270a07f712b78513

You'll notice that I added a debugPrint to log the snapshot state during rebuilds. Once the initial pages have loaded, flicking to scroll leads to the following debugPrints (ignore the 'Showing loading indicator' lines, that just shows when the loading indicator is built):

I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore true hasMore true
I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore false hasMore true
I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore true hasMore true
I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore false hasMore true
I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore true hasMore true
I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore false hasMore true
I/flutter (26367): isFetching false isFetchingMore true hasMore true
I/flutter (26367): isFetching false isFetchingMore false hasMore true
I/flutter (26367): isFetching false isFetchingMore true hasMore true
I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore false hasMore true
I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore true hasMore true
I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore false hasMore false
I/flutter (26367): isFetching false isFetchingMore false hasMore true
I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore true hasMore true
I/flutter (26367): Showing loading indicator
I/flutter (26367): isFetching false isFetchingMore false hasMore false
I/flutter (26367): isFetching false isFetchingMore false hasMore true
I/flutter (26367): isFetching false isFetchingMore true hasMore true
I/flutter (26367): isFetching false isFetchingMore false hasMore false
I/flutter (26367): isFetching false isFetchingMore false hasMore true

It seems to work well although given that it loads very fast it's hard to be sure if there is any flickering or not. I was surprised to see that hasMore is sometimes false during the flicking, I would've expected it to always be true as there are more pages 🤷 .

In any case this seems to work, it could be worth adding to the example app or the documentation.