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.

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

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

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

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

PIG208 Dec 17, 2024

PIG208 Dec 17, 2024

PIG208 Dec 17, 2024

PIG208 Dec 17, 2024

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

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

PIG208 Dec 17, 2024

Choose a reason for hiding this comment

PIG208 Dec 17, 2024

Choose a reason for hiding this comment

PIG208 Dec 17, 2024

Choose a reason for hiding this comment

PIG208 Dec 17, 2024

Choose a reason for hiding this comment

shivanshsharma13 commented Dec 1, 2024 •

edited

Loading