-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add loading indicator for the image compression #483
Conversation
Deploying with Cloudflare Pages
|
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.
looks good to me.
.then((base64Strings) => { | ||
setInputImageUrls((prevImageUrls) => [...prevImageUrls, ...base64Strings]); | ||
}) | ||
.catch((err) => { |
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.
do you still need add the catch err in your processImages function?
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.
Yeah, the processImages function has catch for handling errors, do you mean we can remove it?
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.
Keep it please. We don't want to fail and crash, or fail silently.
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.
Yeah, the processImages function has catch for handling errors, do you mean we can remove it?
sorry, github folded that part, I thought you miss it.
It looks good to me!
Explanation
I set the
imageUrl
to empty string firstly inside the process image functions and then update it with real url result when compression is done. If it is empty, thespinner
in a square box will be rendered as loading status.Result
Fixes: #467