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.

Sign up for GitHub

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

lightbox: Added user's avatar in the lightbox app bar #1092

Merged
merged 1 commit into from
Jan 7, 2025
+29 −15
Merged

lightbox: Added user's avatar in the lightbox app bar #1092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 18 additions & 15 deletions lib/widgets/lightbox.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,21 +171,24 @@ class _LightboxPageLayoutState extends State<_LightboxPageLayout> {
backgroundColor: appBarBackgroundColor,
shape: const Border(), // Remove bottom border from [AppBarTheme]
elevation: appBarElevation,

// TODO(#41): Show message author's avatar
title: RichText(
text: TextSpan(children: [
TextSpan(
text: '${widget.message.senderFullName}\n',

// Restate default
style: themeData.textTheme.titleLarge!.copyWith(color: appBarForegroundColor)),
TextSpan(
text: timestampText,

// Make smaller, like a subtitle
style: themeData.textTheme.titleSmall!.copyWith(color: appBarForegroundColor)),
])),
title: Row(children: [
Avatar(size: 36, borderRadius: 36 / 8, userId: widget.message.senderId),
const SizedBox(width: 8),
Copy link
Member

Choose a reason for hiding this comment

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

From the screenshots in the thread, this 8px spacing looks kind of crowded to me. Is there a source in particular you took this value from?

What does it look like if it's 12px, or 16px?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I took the reference from Profile page (_UserWidget in Profile.dart) also (_MentionAutocompleteItem in autocomplete.dart)

For the suggested changes, please verify this..

spacing Image
12px zulip_mobile
16px flutter_image

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I took the reference from Profile page (_UserWidget in Profile.dart) also (_MentionAutocompleteItem in autocomplete.dart)

Ah, very helpful, thanks.

And in those screenshots I see 16px is definitely too wide, and 12px already looks a little loose. Let's go with 8px, then. At some point our designer will get to this screen (he has plenty of others in his queue first), and then we'll probably change a bunch of aspects including these details, but that should be fine until then.

Expanded(
child: RichText(
Comment on lines +175 to +178
Copy link
Member

Choose a reason for hiding this comment

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

Is there an updated screenshot of what the current version looks like? I see one at the top of the PR but I'm not sure it's up to date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, This is the updated screenshot...

latest_changes

text: TextSpan(children: [
TextSpan(
text: '${widget.message.senderFullName}\n',

// Restate default
style: themeData.textTheme.titleLarge!.copyWith(color: appBarForegroundColor)),
TextSpan(
text: timestampText,

// Make smaller, like a subtitle
style: themeData.textTheme.titleSmall!.copyWith(color: appBarForegroundColor)),
]))),
]),
bottom: widget.buildAppBarBottom(context));
}

Expand Down
11 changes: 11 additions & 0 deletions test/widgets/lightbox_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,17 @@ void main() {
debugNetworkImageHttpClientProvider = null;
});

testWidgets('app bar shows sender avatar', (tester) async {
prepareBoringImageHttpClient();
final message = eg.streamMessage(sender: eg.otherUser);
await setupPage(tester, message: message, thumbnailUrl: null);

final avatar = tester.widget<Avatar>(find.byType(Avatar));
check(avatar.userId).equals(message.senderId);

debugNetworkImageHttpClientProvider = null;
});

testWidgets('header and footer hidden and shown by tapping image', (tester) async {
prepareBoringImageHttpClient();
final message = eg.streamMessage(sender: eg.otherUser);
Expand Down