-
Notifications
You must be signed in to change notification settings - Fork 52
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 better error handling for manual scanning of alt text or image tags #231
Conversation
…rror objects if anything goes wrong. Modify our JS that makes these API requests to ensure if something goes wrong, we output that error message to the user and remove the spinner.
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 just have a question about retrying scanning, otherwise, this is looking good to me!
src/js/media.js
Outdated
const errorObj = get( error, 'responseJSON', { | ||
code: 'unknown_error', | ||
message: __( 'An unknown error occurred.', 'classifai' ), | ||
} ); | ||
spinner.style.display = 'none'; | ||
spinner.classList.remove( 'is-active' ); | ||
button.textContent = __( 'Error', 'classifai' ); | ||
errorContainer.style.display = 'inline-block'; | ||
errorContainer.textContent = errorObj.message; |
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.
How do users retry scanning after failing (due to network issue for example)?
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.
@dinhtungdu Thanks for the review! Just pushed a fix for this, so if an error happens, we make sure the button is no longer disabled, so it can be clicked again
…escan can happen again
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.
LGTM 🎉
Description of the Change
Make sure our requests to generate alt text or image tags return a WP_Error object if something goes wrong. Currently we are returning those in a few instances but not in every instance.
In our javascript that makes these requests, if the response fails, check to see if we have error details and if so, show that error message. If we don't have error details, output a default error message. Also, make sure the spinner is removed so it doesn't appear like things are still processing.
Alternate Designs
None
Benefits
If a request fails, we provide feedback to the user about what went wrong.
Possible Drawbacks
None
Verification Process
Checklist:
Applicable Issues
#230