avatar: Show placeholder on image load error #1882
Conversation
vinodsingh-dev-gsoc
force-pushed
the
fix/avatar-placeholder-1558
branch
from
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. |
vinodsingh-dev-gsoc
force-pushed
the
fix/avatar-placeholder-1558
branch
from
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! 😊 |
vinodsingh-dev-gsoc
force-pushed
the
fix/avatar-placeholder-1558
branch
from
3f69209 to
0b69fea
Compare
vinodsingh-dev-gsoc
force-pushed
the
fix/avatar-placeholder-1558
branch
from
0b69fea to
aa123be
Compare
As you can see in the "After" screenshot, the fix ensures that a placeholder is now displayed whenever an avatar URL is null or fails to load, instead of showing a blank space. This provides a clearer and more consistent UI for the user. |
test/widgets/user_test.dart
Outdated
| class MockHttpClient extends Fake implements HttpClient { | ||
| @override | ||
| Future<HttpClientRequest> getUrl(Uri url) async { | ||
| throw const SocketException('test error'); | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks like the kind of thing that FakeImageHttpClient is for, in test/test_images.dart. Can we use that?
Oh! Or maybe add a boolean success param to prepareBoringImageHttpClient in that file (defaulting to true), and pass false in this case to make it prepare an HttpStats.notFound response?
test/widgets/user_test.dart
Outdated
| await tester.pump(); // Image fails to load | ||
| expect(find.byIcon(ZulipIcons.person), findsOneWidget); | ||
|
|
||
| expect(find.byType(DecoratedBox), findsOneWidget); |
There was a problem hiding this comment.
This seems like it could easily find a DecoratedBox that's not related to the behavior we're testing.
There was a problem hiding this comment.
How about checking the icon in this way:
check(find.descendant(
of: find.byType(AvatarImage),
matching: find.byIcon(ZulipIcons.person),
)).findsOne();(adding the import 'package:flutter_checks/flutter_checks.dart'; at the top as needed)
|
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
errorBuilderto theAvatarImagewidget and includes a test case to verify the new behavior.Co-authored-by: Chris Bobbe [email protected]