autocomplete: Implement new design for @-mention autocomplete items #995

fombalang · 2024-10-11T20:34:47Z

Hello everyone,

This PR implements the updated design for the @-mention autocomplete items as detailed in this Figma File
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3859-3131&m=dev

I added new contextMenuItemLabel and contextMenuItemMeta color variables to the designVariables class from the design.
I also had to manually set the line height to match the design exactly.

As Greg described here, the autocomplete item only shows the user's real email address when the user has allowed their email to be visible, if not it doesn't show anything. I used the _getDisplayEmailFor() method from the profile page to get the user's real email. I am not sure if it is a good idea to have a copy of the same method here, maybe we should find a way to not have multiple copies of the same method.

I updated autocomplete tests to also check if the email(metaData) is also shown as the updated design now shows extra metadata.

I attempted to add another test to verify if the email is not shown when the user's emailAddressVisibility != emailAddressVisibility.everyone, while doing so I learned a bit more about how the app manages users and data, but to be honest I couldn't get it to work properly, and I did not want to massively change up the current test file. Still trying to wrap my head around that.
This also leads me to a bug I think, which I discovered in the example_data.dart initialSnapshot method where some of the arguments from the intialSnapshot method aren't being passed down to the method, I think someone missed that when setting up the method.

Please review when you can, thank you

Fixes: #913

chrisbobbe

Thanks! Comments below.

In the Figma:

The gap between the avatar photo and the text is 6px, not 8px
The outer padding is top: 4, right: 8, bottom: 4, left: 4 (not top: 8, right: 16, bottom: 8, left: 16) Since left and right are unequal, we should use EdgeInsetsDirectional for this.
Please post screenshots showing the "…" effect when the name or metadata text are very long (not sure this is implemented in this revision)
The avatar photo is a 36px rounded square with 4px border radius (not a 32px square with 3px border radius)
Our designer doesn't want the "ink splash" effect on buttons. We should remove the InkWell widget and implement the on-pressed appearance specified here:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3732-28939&node-type=symbol&m=dev

chrisbobbe · 2024-10-11T23:51:33Z

lib/widgets/theme.dart

+  final Color contextMenuItemLabel;
+  final Color contextMenuItemMeta;


Since these are variables in the Figma, they don't belong in this section, which is marked

// Not named variables in Figma; taken from older Figma drafts, or elsewhere.

They should appear alphabetically in the section at the top:

final Color background; final Color bgCounterUnread; final Color bgTopBar; // [etc.]

When moving them there, please also update the other places these names appear in the class, such as the lerp method.

chrisbobbe · 2024-10-11T23:54:41Z

lib/widgets/theme.dart

@@ -167,6 +169,8 @@ class DesignVariables extends ThemeExtension<DesignVariables> {
      // TODO(design-dark) need proper dark-theme color (this is ad hoc)
      subscriptionListHeaderText: const HSLColor.fromAHSL(1.0, 240, 0.1, 0.75).toColor(),
      unreadCountBadgeTextForChannel: Colors.white.withValues(alpha: 0.9),
+      contextMenuItemLabel: const Color(0xffDFE1E8),


I think we more often use lowercase letters in these.

chrisbobbe · 2024-10-12T00:00:45Z

lib/widgets/autocomplete.dart

+  /// The given user's real email address, if known, for displaying in the UI.
+  ///
+  /// Returns null if self-user isn't able to see [user]'s real email address.
+  String? _getDisplayEmailFor(User user, {required PerAccountStore store}) {


Yes, as you suggested in the PR description, we should avoid copy-pasting this complicated logic from somewhere else. If we have some complicated logic written twice, and it needs to be updated, there's a risk that we'll update one place and forget the other.

Maybe a good place for a reusable function is lib/api/model/model.dart, below the User class? That refactor should be in an NFC prep commit.

chrisbobbe · 2024-10-12T00:04:03Z

lib/widgets/autocomplete.dart

        avatar = Avatar(userId: userId, size: 32, borderRadius: 3);
-        label = PerAccountStoreWidget.of(context).users[userId]!.fullName;
+        label = user!.fullName;
+        metaData = _getDisplayEmailFor(user, store: store) ?? '';


nit: Since "metadata" is one word, the variable should be called metadata.

Also, let's make it nullable, so String? metadata;, so its empty value is null instead of the empty string.

chrisbobbe · 2024-10-12T00:05:52Z

lib/widgets/autocomplete.dart

    switch (option) {
      case UserMentionAutocompleteResult(:var userId):
+        final store = PerAccountStoreWidget.of(context);
+        final user = store.users[userId];


Suggested change

final user = store.users[userId];

final user = store.users[userId]!;

instead of putting the ! later, when we try to access .fullName.

chrisbobbe · 2024-10-12T00:16:40Z

lib/widgets/autocomplete.dart

+            Column(
+              mainAxisSize: MainAxisSize.min,
+              crossAxisAlignment: CrossAxisAlignment.start,
+              children: [
+              Text(label, style: TextStyle(fontSize: 18,  height: 1.1, color: designVariables.contextMenuItemLabel) //Creates a line height equavalent to ~20
+                  .merge(weightVariableTextStyle(context, wght: 600))),
+              Visibility(
+                visible: metaData.isNotEmpty,
+                child: Text(metaData, style: TextStyle(height: 1.14, color: designVariables.contextMenuItemMeta)), //Creates a line height equavalent to ~16
+              )])])));


indentation and line lengths:

Suggested change

Column(

mainAxisSize: MainAxisSize.min,

crossAxisAlignment: CrossAxisAlignment.start,

children: [

Text(label, style: TextStyle(fontSize: 18, height: 1.1, color: designVariables.contextMenuItemLabel) //Creates a line height equavalent to ~20

.merge(weightVariableTextStyle(context, wght: 600))),

Visibility(

visible: metaData.isNotEmpty,

child: Text(metaData, style: TextStyle(height: 1.14, color: designVariables.contextMenuItemMeta)), //Creates a line height equavalent to ~16

)])])));

Column(

mainAxisSize: MainAxisSize.min,

crossAxisAlignment: CrossAxisAlignment.start,

children: [

Text(

style: TextStyle(

fontSize: 18,

height: 1.1, // Creates a line height equavalent to ~20

color: designVariables.contextMenuItemLabel,

)

.merge(weightVariableTextStyle(context, wght: 600)),

label),

Visibility(

visible: metaData.isNotEmpty,

child: Text(

style: TextStyle(

height: 1.14, // Creates a line height equavalent to ~16

color: designVariables.contextMenuItemMeta,

),

metaData)),

]),

])));

chrisbobbe · 2024-10-12T00:18:33Z

lib/widgets/autocomplete.dart

+              mainAxisSize: MainAxisSize.min,
+              crossAxisAlignment: CrossAxisAlignment.start,
+              children: [
+              Text(label, style: TextStyle(fontSize: 18,  height: 1.1, color: designVariables.contextMenuItemLabel) //Creates a line height equavalent to ~20


See other places where we set height; here, we would express it as 20 / 18.

chrisbobbe · 2024-10-12T00:18:56Z

lib/widgets/autocomplete.dart

+                  .merge(weightVariableTextStyle(context, wght: 600))),
+              Visibility(
+                visible: metaData.isNotEmpty,
+                child: Text(metaData, style: TextStyle(height: 1.14, color: designVariables.contextMenuItemMeta)), //Creates a line height equavalent to ~16


height: 16 / 14

Also, this should have an explicit fontSize: 14. Even if that matches the default, let's be explicit here.

chrisbobbe · 2024-10-12T00:26:44Z

lib/widgets/autocomplete.dart

+              children: [
+              Text(label, style: TextStyle(fontSize: 18,  height: 1.1, color: designVariables.contextMenuItemLabel) //Creates a line height equavalent to ~20
+                  .merge(weightVariableTextStyle(context, wght: 600))),
+              Visibility(


Instead of Visibility, how about using Dart's "collection if" feature:

if (metaData.isNotEmpty) Text(/* … */),

, but combined with my earlier suggestions it would be

if (metadata != null) Text(/* … */),

gnprice · 2024-10-12T00:51:22Z

This also leads me to a bug I think, which I discovered in the example_data.dart initialSnapshot method where some of the arguments from the intialSnapshot method aren't being passed down to the method, I think someone missed that when setting up the method.

Sure, this would be good to fix — please go ahead and add a commit with that fix if you think you already see how to do it, or start a thread in #mobile-team and we can discuss in more detail.

fombalang · 2024-10-14T16:09:37Z

Okay, thank you for the review, I will make the changes.
For the changes such as the bug and the refactor of the email logic, should I add those into this PR, or should it be in a different PR?

gnprice · 2024-10-14T17:13:46Z

If it's needed for the other work you're doing in this PR, then a commit in this PR is a good way to do it.

If it's not related to what's in this PR, then a separate PR is best.

fombalang · 2024-10-14T17:42:29Z

If it's needed for the other work you're doing in this PR, then a commit in this PR is a good way to do it.

If it's not related to what's in this PR, then a separate PR is best.

Okay

fombalang · 2024-10-14T20:32:28Z

Our designer doesn't want the "ink splash" effect on buttons. We should remove the InkWell widget and implement the on-pressed appearance specified here:

About the splash, I kept the inkwell widget, because a gesture detector doesn't show splashes, but I removed the sparkle effect, and left just the color change on tap. I also added a border radius in the splash as shown on the design, it's a bit difficult to see here but it is there. Would this work according to the design?

screen-20241014-204642.mp4

fombalang · 2024-10-14T20:39:15Z

I also did the updates for the case where the user has a really long name or email.

Before

After

Implemented new design for @-mention autocomplete items. Added new `contextMenuItemLabel` and `contextMenuItemMeta` color variables to `designVariables` class. Updated autocomplete tests. Fixes: zulip#913

chrisbobbe · 2024-10-15T18:33:57Z

Would this work according to the design?

If your code causes any noticeable differences from the design, please identify them and say why you think they are OK. This will shorten the path to deciding if the code will work. :)

It looks like you haven't pushed your latest revision to GitHub. When it's ready for another review, please do that, and comment saying that it's ready for review.

fombalang · 2024-10-15T19:33:48Z

It looks like you haven't pushed your latest revision to GitHub. When it's ready for another review, please do that, and comment saying that it's ready for review.

No, I haven't yet.

That is because I have been facing some problems with the ComposeAutocomplete test, I found that changing the dimensions of the avatar widget from 32 to 36 causes the test to fail. Not only that I found that there was a threshold of sizes for which the test passed, 32 -33 are good, and from 34 upward the test fails.

And I have looked at the test, and tried to do some searching as to why it might be failing, and I haven't succeeded. I did not see any obvious place where the avatar widget size was checked in the test. I also thought it might be because of an outdated golden test snapshot, but I could not find any references to a snapshot file. So I have been confused and stumped.

I was just about to comment on that before seeing your comment, wondering if anyone can help point me in the right direction.

gnprice · 2024-10-15T19:36:32Z

If you push the code here, and then start a conversation in #mobile-dev-help with the test failure you're seeing, that will probably be the most efficient way to get help investigating it.

fombalang · 2024-10-15T20:07:30Z

If you push the code here, and then start a conversation in #mobile-dev-help with the test failure you're seeing, that will probably be the most efficient way to get help investigating it.

Okay, I will do that

Moved getDisplayEmailFor method which gets the user's real email address to lib/api/model/model.dart and refactored usages to remove duplication.

fombalang · 2024-10-16T04:38:27Z

It looks like you haven't pushed your latest revision to GitHub. When it's ready for another review, please do that, and comment saying that it's ready for review.

I have pushed my changes, please review

chrisbobbe · 2024-10-16T22:26:52Z

If you push the code here, and then start a conversation in #mobile-dev-help with the test failure you're seeing, that will probably be the most efficient way to get help investigating it.

Thanks for starting that conversation. Here's a link, so people reading this will know the conversation is happening and where to find it: https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/ComposeAutocomplete.20test.20failure.20help/near/1963058

chrisbobbe self-assigned this Oct 12, 2024

chrisbobbe requested changes Oct 12, 2024

View reviewed changes

autocomplete: Implement new design for @-mention autocomplete items

2bb8f2a

Implemented new design for @-mention autocomplete items. Added new `contextMenuItemLabel` and `contextMenuItemMeta` color variables to `designVariables` class. Updated autocomplete tests. Fixes: zulip#913

user [nfc]: Move getDisplayEmailFor method to lib/api/model/model.dart

e107e03

Moved getDisplayEmailFor method which gets the user's real email address to lib/api/model/model.dart and refactored usages to remove duplication.

fombalang force-pushed the autocomplete-results-list branch from 17aea85 to e107e03 Compare October 15, 2024 20:25

This comment was marked as resolved.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

autocomplete: Implement new design for @-mention autocomplete items #995

autocomplete: Implement new design for @-mention autocomplete items #995

fombalang commented Oct 11, 2024

chrisbobbe left a comment

chrisbobbe Oct 11, 2024

chrisbobbe Oct 11, 2024

chrisbobbe Oct 12, 2024

chrisbobbe Oct 12, 2024

chrisbobbe Oct 12, 2024

chrisbobbe Oct 12, 2024

chrisbobbe Oct 12, 2024

chrisbobbe Oct 12, 2024

chrisbobbe Oct 12, 2024

chrisbobbe Oct 12, 2024

chrisbobbe Oct 12, 2024

gnprice commented Oct 12, 2024

fombalang commented Oct 14, 2024

gnprice commented Oct 14, 2024

fombalang commented Oct 14, 2024

fombalang commented Oct 14, 2024

fombalang commented Oct 14, 2024

chrisbobbe commented Oct 15, 2024

fombalang commented Oct 15, 2024 •

edited

Loading

gnprice commented Oct 15, 2024

fombalang commented Oct 15, 2024

fombalang commented Oct 16, 2024

This comment was marked as resolved.

This comment was marked as resolved.

chrisbobbe commented Oct 16, 2024

		final Color contextMenuItemLabel;
		final Color contextMenuItemMeta;

	final user = store.users[userId];
	final user = store.users[userId]!;

autocomplete: Implement new design for @-mention autocomplete items #995

Are you sure you want to change the base?

autocomplete: Implement new design for @-mention autocomplete items #995

Conversation

fombalang commented Oct 11, 2024

chrisbobbe 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

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

gnprice commented Oct 12, 2024

fombalang commented Oct 14, 2024

gnprice commented Oct 14, 2024

fombalang commented Oct 14, 2024

fombalang commented Oct 14, 2024

fombalang commented Oct 14, 2024

chrisbobbe commented Oct 15, 2024

fombalang commented Oct 15, 2024 • edited Loading

gnprice commented Oct 15, 2024

fombalang commented Oct 15, 2024

fombalang commented Oct 16, 2024

This comment was marked as resolved.

This comment was marked as resolved.

chrisbobbe commented Oct 16, 2024

fombalang commented Oct 15, 2024 •

edited

Loading