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

fix: await the promise to be able to catch its error and a toast #89

Merged

Conversation

bpingris
Copy link
Contributor

Summary

hello! this PR fixes the toast.promise method with a rejected promise where it is never catch as the promise was not awaited
this resulted in the toast staying in the loading state forever

Test plan

yarn example start and test the two buttons:

  • Toast with a successful promise
  • Toast with a failed promise

Copy link
Owner

@gunnartorfis gunnartorfis left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and well spotted @bpingris 👏

@gunnartorfis gunnartorfis merged commit 02e3915 into gunnartorfis:main Sep 14, 2024
3 checks passed
@gunnartorfis
Copy link
Owner

@bpingris I noticed that multiple toasts are shown once the promise is resolved/rejected. I'll have to revert this PR for now but would be great if you could take a look at it. Is that not happening on your end?

Screen.Recording.2024-09-14.at.09.03.57.mov

@bpingris
Copy link
Contributor Author

bpingris commented Sep 14, 2024

oh yes you're right!
I think I saw it even before starting to implement the promise fix, it seems to be because of the configuration of the <Toaster> in the example app.
while I was trying the PR I saw the issue and just removed every option and I kept it as <Toaster /> without any extra prop

I will still check again that this not related to my PR!

@gunnartorfis
Copy link
Owner

Oh!! You're right, I was too quick on my feet here - it is not related to your PR. I'll revert the reversion 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants