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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+22 −16
Open

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
32 changes: 17 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,23 @@ 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?

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.

text: TextSpan(
children: [
Comment on lines +179 to +180
Copy link
Member

Choose a reason for hiding this comment

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

nit: preserve formatting of code you're not changing

Suggested change
text: TextSpan(
children: [
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)),
]))),
]),
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
]),
]),

The close-bracket gets aligned with the start of the line the open-bracket is on. Same thing as for braces {} and parens ().

bottom: widget.buildAppBarBottom(context));
}

Expand Down
6 changes: 5 additions & 1 deletion test/widgets/lightbox_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,16 @@ void main() {
debugNetworkImageHttpClientProvider = null;
});

testWidgets('app bar shows sender name and date', (tester) async {
testWidgets('app bar shows sender avatar, name and date', (tester) async {
prepareBoringImageHttpClient();
final timestamp = DateTime.parse("2024-07-23 23:12:24").millisecondsSinceEpoch ~/ 1000;
final message = eg.streamMessage(sender: eg.otherUser, timestamp: timestamp);
await setupPage(tester, message: message, thumbnailUrl: null);

// Check Avatar properties
final avatar = tester.widget<Avatar>(find.byType(Avatar));
check(avatar.userId).equals(message.senderId);
Comment on lines +245 to +247
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this a new separate test case (a separate call to testWidgets), rather than adding complexity to an existing test case.

That way the new test case doesn't need to worry about e.g. the timestamp of the message, and the existing test case doesn't get this added logic sitting in between things like the setup that prepares the timestamp's value, and the check that says how it should look. For the test to make sense, the reader needs to see those things together and compare them, so it's best to minimize unrelated things coming in between and separating them.

Comment on lines +245 to +247
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Check Avatar properties
final avatar = tester.widget<Avatar>(find.byType(Avatar));
check(avatar.userId).equals(message.senderId);
final avatar = tester.widget<Avatar>(find.byType(Avatar));
check(avatar.userId).equals(message.senderId);

This comment doesn't say anything the code just under it doesn't already say just as clearly.

It's also wrong — this code checks only one property.

I believe this illustrates the classic way that comments tend to become wrong over time, which is that you edited the code (#1092 (comment)) and didn't update the comment. If that can happen when you yourself are editing the code, within the same PR where you wrote the comment in the first place, you can imagine it happens all the time when other people are updating the code months and years later.

The fact that comments routinely become wrong is a major reason to leave out comments except those that pull their weight by really adding something that's not easy to see by just looking at the code.


// We're looking for a RichText, in the app bar, with both the
// sender's name and the timestamp.
Copy link
Member

Choose a reason for hiding this comment

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

nit: the comment was specifically for explaining what RichText was used for. Let's leave it as-is

Copy link
Member

Choose a reason for hiding this comment

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

The comment belongs to the part started with final labelTextWidget, so it should come before that and replace // Check sender name and timestamp.

final labelTextWidget = tester.widget<RichText>(
Expand Down
Loading