-
Notifications
You must be signed in to change notification settings - Fork 341
avatar: Show placeholder on image load error #1882
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
base: main
Are you sure you want to change the base?
avatar: Show placeholder on image load error #1882
Conversation
60b16e2
to
97f92e7
Compare
Thanks for the PR! Please post before/after screenshots of the change. Please also change the commit sequence so the tests are added in the same commit that adds the behavior being tested. |
97f92e7
to
3f69209
Compare
Hi chrisbobbe! Thanks a lot for the detailed feedback. I'm currently working on your suggestions. I've gathered the before/after screenshots and am now in the process of squashing the commits and fixing the test suite to make the PR perfect. I'll push the updated changes shortly! 😊 |
3f69209
to
0b69fea
Compare
0b69fea
to
aa123be
Compare
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! Small comments below.
Also please update the commit message to mention the issue being fixed, according to our style. |
Co-authored-by: Chris Bobbe <[email protected]>
Co-authored-by: Chris Bobbe <[email protected]>
I see you've pushed some changes, but I'm not sure if you intend them to be ready for review; I assume you'll comment here when ready. 🙂 |
Hi @chrisbobbe, Thanks for the ping, and my apologies for the delay in updating you here. I've addressed your suggestions and pushed the commits a few days ago. The PR should now be ready for another look. Eager to hear your feedback. Thank you for your guidance! |
Please see our guide on how to demonstrate visual changes: https://zulip.readthedocs.io/en/latest/contributing/reviewable-prs.html#demonstrating-visual-changes. The before/after screenshots should be minimally different. I would also like to see both placeholders and regular avatars in the same screenshot, and dark theme as well as light. |
Hi @alya, thanks for the detailed feedback! That makes perfect sense. I will generate the new screenshots with both themes and mixed avatars as you suggested and will post them here shortly. |
Fixes #1558.
This change improves the user experience by displaying a consistent placeholder when an avatar image fails to load from its URL.
Adds an
errorBuilder
to theAvatarImage
widget and includes a test case to verify the new behavior.Co-authored-by: Chris Bobbe [email protected]