search

stargazing-dino / async_button_builder

A helper builder to create loading buttons in Flutter
https://pub.dev/packages/async_button_builder
MIT License
29 stars 9 forks source link

Added error and stack trace parameters to onError #35

Open jonjomckay opened 2 years ago

jonjomckay commented 2 years ago

This adds the thrown error and stackTrace parameters to the onError callback.

It's quite useful to have these passed in, so the caller can e.g. show a SnackBar with some of the error details.

stargazing-dino commented 2 years ago

Man, I was not paying attention when I added that callback. Good catch !

I think there might already be a flutter typedef that is (Object? error, Stacktrace? stack) but I might be wrong. Anyways, will look at this when I get home. Also, you might want to pull to resolve conflicts as I merged your other pr

stargazing-dino commented 2 years ago

Currently our callbacks (onSuccess, onError) fire at the end of the errorDuration/successDuration so they might mislead the user to think they'll fire as soon as an error is encountered. I'm not sure when I added these callbacks but I think they should be renamed to onSuccessCompleted or onErrorShown etc to show they're at the end of error/loading state. For immediately telling if it's an error or success, you could do it in the async onPressed no?

What are your thoughts on this

jonjomckay commented 2 years ago

I couldn't find a typedef in Flutter itself that defines (Object? error, Stacktrace? stack), but I'm happy to change it if you can point me towards it :)

Do onError and onSuccess need to fire after the defined durations, or can they fire straight away? I see the code is used to wait for a bit before running the callback, then resetting the icon back to the idle one, but they could be split into two?

If it helps, I'm using onError and onSuccess to display snackbars with appropriate messages after the button action is performed. For my use case, I'd prefer the callbacks to run as soon as an error or success happens, but I'd still want the button icon to change gracefully, like it currently does. The reason I'd like to use these callbacks is so I don't have to litter my code with duplicate-y try/catch statements, and can instead just pass a method reference.

I'll leave it in your hands to think over, but let me know if you want me to make any changes!

stargazing-dino commented 2 years ago

Yeah no worries with the typedef. What you have currently looks good.

So I think what is best is to rename the current callbacks we have now to onSuccessShown and onErrorShown. That would show the user those callbacks happen after the specified duration. I imagine these callbacks being used to route users or similar so I think they have utility.

After that, we should add back in onError and onSuccess but so that they're immediate.

                    timer?.cancel();

                    onPressed.call().then((_) {
                      completer.complete();
                      onSuccess?.call();

                      if (mounted) {

I think the docs should better reflect how these callbacks are called as well since what I have currently doesn't make sense.

If you want to do both the changing and additions in this PR or only the renaming, however is fine. Thank you for the help too !