-
Notifications
You must be signed in to change notification settings - Fork 26
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
Updated example and v2 of lazy_load_scrollview #16
base: master
Are you sure you want to change the base?
Conversation
Hey Thanks for this and sorry for the late response. Ill have a look this week! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you contributions so far! I've just added a couple of questions
lib/src/lazy_load_scrollview.dart
Outdated
setState(() => isLoadingAfter = true); | ||
|
||
if (mounted) { | ||
// TODO: Will these need completers to correctly handle errors? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you expect to do with the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's user convenience. I have a package called async_button_builder that wraps a user's callback and handles its various states. I found that if I don't use a completer and their function errors, I can rethrow
but that won't give them the stackTrace. It'll also throw at my callsite of their function too rather than in their original function which was kinda lame. It's a gut assumption right now as I haven't tested, but I think if their load more functions error, the same will happen and they'll get no helpful stacktrace (again, not sure tho). I'll test on weekend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nvm, just checked real quick. It handles the error correctly. I'll remove that TODO:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did forget though that if it errors, it won't call anything afterward which means the loading state won't be reset. Here's before:
await widget.onLoadAfter?.call().timeout(widget.timeout);
setState(() => isLoadingAfter = false);
after:
await widget.onLoadAfter?.call().timeout(widget.timeout).whenComplete(
() {
setState(() => isLoadingAfter = false);
},
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/src/lazy_load_scrollview.dart
Outdated
/// Note, the direct child of this widget should be the [ListView]. If it is | ||
/// not and the [ListView] is further down the tree then it's likely that this | ||
/// will Builder will fail to recognize scroll events unless [ignoreDepth] is | ||
/// set to true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is likely and undesired behaviour, can we add assertions in the constructor to prevent this from happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think I wrote that before the predicate. How does this sound instead:
/// The [Scrollable] should be a direct child of this builder for us to listen
/// to bubbling events. If it is not the direct child, you must further specify
/// a predicate in order to find it in the descendents or, if there are no other
/// scrollables, set [ignoreDepth] to true. If you'd like to avoid a
/// predicate, you may also provide an optional [ScrollController].
I don't think we can do an assertion either of a child of a builder because it'd need to be built first too. But that comment should clear things up.
// FIXME: I don't think this is true | ||
// Scrollbar eats notifications ?? so we'll just use a scroll controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on the concern here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, this was weird. So I think even if I set the predicate to the right layer where the descendant scrollable was, I couldn't get the thing to work (without using a scrollcontroller). I actually think the Scrollbar
is handling the notifications and not allowing them to bubble (by returning false in their NotificationListener
onNotification). Can you confirm that it worked previously with your implementation (wrt Scrollbar specifically)?
receives typo Co-authored-by: Quirijn Groot Bluemink <837393+QuirijnGB@users.noreply.github.com>
I have not forgotten about this! Ill get to it soon |
Hello ! I didn't know of the existence of this package and ended up creating listview notifier similar to this but slightly different. In any case, I'd like to offer it up as a PR. To improve I took both my ideas and your ideas and kind of squashed them together as well as implemented a few things from the issues here.
Here are a few things the new:
Breaking change:
I'd love to get your opinion on this big change. Leaving it as a draft for now