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

shivanshsharma13 · 2024-12-01T07:11:04Z

In the lightbox, the RN app shows the message author's avatar along with their name in the app bar. This change implements the same behavior in the Flutter app, positioning the avatar beside the name/date text in the "start" direction (left in LTR layouts, right in RTL layouts).

The changes:

Add Avatar widget in the lightbox app bar next to the author's name and date
Ensure the avatar is positioned correctly based on the text direction (LTR/RTL)
Maintain consistent sizing and spacing (35px size, 8px right padding)
Handle overflow gracefully for both avatars and names

Screenshot-

Fixes #41

alya · 2024-12-02T17:54:53Z

Thanks @shivanshsharma13 ! Please add screenshots showing your changes to the PR description.

shivanshsharma13 · 2024-12-02T18:01:55Z

Thanks @shivanshsharma13 ! Please add screenshots showing your changes to the PR description.

HI @alya Done!

gnprice · 2024-12-05T22:34:36Z

Thanks for the PR. Before we can review this, it will need (a) tests and (b) to follow our Git style. See our README at: https://github.com/zulip/zulip-flutter#submitting-a-pull-request

gnprice · 2024-12-05T22:40:33Z

Oh also:

The screenshot doesn't show any recipient headers. The thing at the top of the screen (with a back button and the page's title) is the app bar.

A Zulip "recipient header" is something shown inside the message list, potentially many times, as we show a new recipient header at each point where the recipient changes from one message to the next. If you look at a combined feed you'll see many examples. See also our code — we use the term "recipient header" there.
The linked issue is about the lightbox, which this doesn't touch. So this shouldn't be marked as fixing that issue.

shivanshsharma13 · 2024-12-07T10:07:50Z

Hi, sorry was a bit confused. Now I have implemented the correct code. It's ready for review. Adding test for this as well.

chrisbobbe · 2024-12-09T19:25:32Z

Thanks for the PR. Before we can review this, it will need (a) tests and (b) to follow our Git style. See our README at: https://github.com/zulip/zulip-flutter#submitting-a-pull-request

shivanshsharma13 · 2024-12-11T05:08:18Z

Thanks for the PR. Before we can review this, it will need (a) tests and (b) to follow our Git style. See our README at: https://github.com/zulip/zulip-flutter#submitting-a-pull-request

Done the changes!

Wrapped commit message in 68 chars a line
Added the Fixes: format like other commits
Added the test case for the change.

chrisbobbe · 2024-12-11T21:32:22Z

Thanks! How about making it size 36px and border-radius 36 / 8, like in zulip-mobile, instead of 40 and 32 / 8. (Not sure where the 40 and the 32 come from and why they're different from each other.)

Here's a screenshot with those numbers I've suggested:

Also please fix indentation and whitespace to match the project's style, and use lightbox: as the prefix in the summary line, as we do in other lightbox-related commits (use our handy tip for viewing Git history).

shivanshsharma13 · 2024-12-12T10:26:34Z

Hi, thanks for the review! I changed the the Summary line as suggested. For the Avatar I chose to use 40 as it feels more fitting and consistent with the RN app.

And used border radius based on -

borderRadius: 32/8 in Profile page
borderRadius: 3 in message list page

Could you please take a look at the difference between them and share your thoughts?

Size and Border Radius	Image
RN app
Flutter (40 and 32/8)
Flutter (36 and 36/8)

chrisbobbe · 2024-12-12T22:10:42Z

Thanks for those screenshots, although I notice they're a little pixelated.

For the Avatar I chose to use 40 as it feels more fitting and consistent with the RN app.

As I said, the RN app uses 36 and 36 / 8. The lightbox page will likely change when our designer makes a design for it. Temporarily, I think taking these numbers from the RN app is fine.

There's no point using the n / 8 form if n doesn't equal the size, whether that's 36 or 40. I like 36, and it's easy to explain because it just follow the number in zulip-mobile. Greg may have a different preference.

Please choose either:

36 and 36 / 8 or
40 and 40 / 8

Please also update the commit with the other things I requested: #1092 (comment)

shivanshsharma13 · 2024-12-13T21:47:34Z

Hi, Thanks for the detailed explanation. Added the 36 size and 36/8 as per suggestion. Also changed the indentation. It's ready for review.🙂

chrisbobbe · 2024-12-13T22:00:36Z

CI has caught a failing test; please fix that.

shivanshsharma13 · 2024-12-14T03:05:02Z

Sorry, I just missed updating the test file earlier. It's done now. Thanks!

PIG208

Thanks for the update @shivanshsharma13! Left some comments. Also a commit message nit:

 Lightbox: Added user's avatar in the lightbox app bar

to

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

For finding the right prefix, git log --oneline -- lib/widgets/lightbox.dart is handy.

PIG208 · 2024-12-17T23:03:54Z

lib/widgets/lightbox.dart

+                ])
+              ),
+            ),
+          ],
+        ),


nit: Looks like this change expands the parentheses into separate lines. We should fix this.

Suggested change

])

),

),

],

),

]))),

]),

PIG208 · 2024-12-17T23:04:12Z

lib/widgets/lightbox.dart

+          ],
+        ),
+        bottom: widget.buildAppBarBottom(context)
+      );


nit: same issue here

bump (need to move the parenthesis)

PIG208 · 2024-12-17T23:05:49Z

test/widgets/lightbox_test.dart

      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);

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


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

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

PIG208 · 2024-12-17T23:12:26Z

test/widgets/lightbox_test.dart

+      // Check Avatar properties
+      final avatar = tester.widget<Avatar>(find.byType(Avatar));
+      check(avatar.size).equals(36);
+      check(avatar.borderRadius).equals(36 / 8);


The design properties like size and borderRadius are usually verified during reviews. We typically don't need test for these because it only captures a limited scope of bugs and becomes another place to update if the design spec changes.

Thank you for the review! Should I remove the test, or would you suggest modifying it in some way? Looking forward to your guidance! For now I just removed the test for size and border radius

shivanshsharma13 · 2024-12-18T16:44:07Z

Thanks for the detailed review. I acknowledged the suggested changes. Please let me know if anything needs to be changed!🙂

PIG208

Thanks for the update! Left some comments.

PIG208 · 2024-12-18T19:42:05Z

lib/widgets/lightbox.dart

+          ],
+        ),
+        bottom: widget.buildAppBarBottom(context)
+      );


bump (need to move the parenthesis)

PIG208 · 2024-12-18T19:43:47Z

test/widgets/lightbox_test.dart

      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);

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


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

PIG208

One more commit message nit:

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

See the commit style guide linked in our README: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

Commit summaries should be started with an imperative verb:

lightbox: Add user's avatar on the lightbox app bar

PIG208 · 2024-12-19T17:21:21Z

lib/widgets/lightbox.dart

+                    style: themeData.textTheme.titleSmall!.copyWith(color: appBarForegroundColor)),
+                ]))),
+          ]),
+          bottom: widget.buildAppBarBottom(context));


nit: Hmm, didn't notice this until now — bottom is a parameter for AppBar, so it should be at the same indent level as other parameters like title

Yeah sorry for overlooking it before. Thanks

This change updates the lightbox to display the message author's avatar alongside their name and the date in the app bar. The avatar is positioned in the "start" direction (left for LTR layouts, right for RTL layouts) to align with the behavior in the React Native app. Fixes: zulip#41

gnprice

Thanks @shivanshsharma13, and thanks @chrisbobbe and @PIG208 for the previous reviews! Comments below.

gnprice · 2024-12-20T22:55:53Z

test/widgets/lightbox_test.dart

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


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.

gnprice · 2024-12-20T22:56:17Z

lib/widgets/lightbox.dart

+              text: TextSpan(
+                children: [


nit: preserve formatting of code you're not changing

Suggested change

text: TextSpan(

children: [

text: TextSpan(children: [

gnprice · 2024-12-20T22:57:26Z

lib/widgets/lightbox.dart

+          Avatar(size: 36, borderRadius: 36 / 8, userId: widget.message.senderId),
+          const SizedBox(width: 8),
+          Expanded(
+            child: RichText(


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.

gnprice · 2024-12-20T22:58:30Z

lib/widgets/lightbox.dart

-          ])),
+        title: Row(children: [
+          Avatar(size: 36, borderRadius: 36 / 8, userId: widget.message.senderId),
+          const SizedBox(width: 8),


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?

gnprice · 2024-12-20T23:01:12Z

lib/widgets/lightbox.dart

+                    // Make smaller, like a subtitle
+                    style: themeData.textTheme.titleSmall!.copyWith(color: appBarForegroundColor)),
+                ]))),
+          ]),


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 ().

gnprice · 2024-12-20T23:08:01Z

test/widgets/lightbox_test.dart

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


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.

shivanshsharma13 force-pushed the avatar_in_dm branch from 3b20b77 to a1e4aff Compare December 1, 2024 10:31

shivanshsharma13 force-pushed the avatar_in_dm branch from a1e4aff to 38a8e09 Compare December 7, 2024 10:04

shivanshsharma13 changed the title ~~Add avatars and Name to DM recipient headers~~ Show user's avatar in the lightbox app bar Dec 7, 2024

shivanshsharma13 force-pushed the avatar_in_dm branch from 38a8e09 to 15e1ea8 Compare December 11, 2024 05:05

shivanshsharma13 changed the title ~~Show user's avatar in the lightbox app bar~~ lightbox: Added user's avatar in the lightbox app bar Dec 12, 2024

shivanshsharma13 force-pushed the avatar_in_dm branch from 15e1ea8 to 1835cba Compare December 13, 2024 21:45

shivanshsharma13 force-pushed the avatar_in_dm branch from 1835cba to a4c4cf7 Compare December 14, 2024 03:03

PIG208 reviewed Dec 17, 2024

View reviewed changes

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Dec 17, 2024

PIG208 self-assigned this Dec 17, 2024

shivanshsharma13 force-pushed the avatar_in_dm branch from a4c4cf7 to afd311c Compare December 18, 2024 16:37

PIG208 reviewed Dec 18, 2024

View reviewed changes

shivanshsharma13 force-pushed the avatar_in_dm branch from afd311c to fd10604 Compare December 18, 2024 20:09

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Dec 19, 2024

PIG208 reviewed Dec 19, 2024

View reviewed changes

PIG208 requested a review from gnprice December 19, 2024 17:25

PIG208 assigned gnprice and unassigned PIG208 Dec 19, 2024

shivanshsharma13 force-pushed the avatar_in_dm branch from fd10604 to e094f50 Compare December 19, 2024 18:15

gnprice reviewed Dec 20, 2024

View reviewed changes

-                ])
-              ),
-            ),
-          ],
-        ),
+                ]))),
+          ]),

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

Are you sure you want to change the base?

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

Conversation

shivanshsharma13 commented Dec 1, 2024 • edited Loading

alya commented Dec 2, 2024

shivanshsharma13 commented Dec 2, 2024

gnprice commented Dec 5, 2024

gnprice commented Dec 5, 2024

shivanshsharma13 commented Dec 7, 2024

chrisbobbe commented Dec 9, 2024

shivanshsharma13 commented Dec 11, 2024

chrisbobbe commented Dec 11, 2024

shivanshsharma13 commented Dec 12, 2024

chrisbobbe commented Dec 12, 2024

shivanshsharma13 commented Dec 13, 2024

chrisbobbe commented Dec 13, 2024

shivanshsharma13 commented Dec 14, 2024

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshsharma13 Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

shivanshsharma13 commented Dec 18, 2024

PIG208 left a comment

Choose a reason for hiding this comment

PIG208 Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivanshsharma13 commented Dec 1, 2024 •

edited

Loading

PIG208 Dec 18, 2024 •

edited

Loading

shivanshsharma13 Dec 18, 2024 •

edited

Loading

PIG208 Dec 18, 2024 •

edited

Loading