autocomplete: Implement new design for @-mention autocomplete items #995
Conversation
There was a problem hiding this comment.
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
EdgeInsetsDirectionalfor 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
InkWellwidget 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
lib/widgets/theme.dart
Outdated
| final Color contextMenuItemLabel; | ||
| final Color contextMenuItemMeta; |
There was a problem hiding this comment.
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.
lib/widgets/theme.dart
Outdated
| @@ -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), | |||
There was a problem hiding this comment.
I think we more often use lowercase letters in these.
lib/widgets/autocomplete.dart
Outdated
| /// 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}) { |
There was a problem hiding this comment.
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.
lib/widgets/autocomplete.dart
Outdated
| avatar = Avatar(userId: userId, size: 32, borderRadius: 3); | ||
| label = PerAccountStoreWidget.of(context).users[userId]!.fullName; | ||
| label = user!.fullName; | ||
| metaData = _getDisplayEmailFor(user, store: store) ?? ''; |
There was a problem hiding this comment.
nit: Since "metadata" is one word, the variable should be called metadata.
There was a problem hiding this comment.
Also, let's make it nullable, so String? metadata;, so its empty value is null instead of the empty string.
lib/widgets/autocomplete.dart
Outdated
| switch (option) { | ||
| case UserMentionAutocompleteResult(:var userId): | ||
| final store = PerAccountStoreWidget.of(context); | ||
| final user = store.users[userId]; |
There was a problem hiding this comment.
| final user = store.users[userId]; | |
| final user = store.users[userId]!; |
instead of putting the ! later, when we try to access .fullName.
lib/widgets/autocomplete.dart
Outdated
| 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 | ||
| )])]))); |
There was a problem hiding this comment.
indentation and line lengths:
| 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)), | |
| ]), | |
| ]))); |
lib/widgets/autocomplete.dart
Outdated
| 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 |
There was a problem hiding this comment.
See other places where we set height; here, we would express it as 20 / 18.
lib/widgets/autocomplete.dart
Outdated
| .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 |
There was a problem hiding this comment.
Also, this should have an explicit fontSize: 14. Even if that matches the default, let's be explicit here.
lib/widgets/autocomplete.dart
Outdated
| 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( |
There was a problem hiding this comment.
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(/* … */),
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. |
|
Okay, thank you for the review, I will make the changes. |
|
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 |
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 |
|
I also did the updates for the case where the user has a really long name or email. Before After |
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. |
No, I haven't yet. That is because I have been facing some problems with the 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. |
|
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 |
fombalang
force-pushed
the
autocomplete-results-list
branch
from
17aea85 to
e107e03
Compare
I have pushed my changes, please review |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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 |
Hello |
|
Thanks; I'll add this to my review queue. 🙂 I see Greg has replied in that discussion about the test failures. Please comment here when those failures are resolved and this PR is ready for another review. 🙂 |
fombalang
force-pushed
the
autocomplete-results-list
branch
from
e107e03 to
5b85e89
Compare
Hello I did an interactive rebase and merged the test fix to the previous commit before the latest one, I think it caused it to not run the pipeline since the latest commit is still the same one from before, maybe we might have to trigger it manually. |
|
It's puzzling that GitHub didn't run the CI checks on the latest revision. It's not because the commits are the same; they're different. Even though the last commit has the same changes in it as the last commit of the previous revision, it's still a different commit as far as Git is concerned because it has a different history. You can see here:
that the latest revision's tip commit is 5b85e89 and the revision before that was e107e03. Anyway, we can run the tests locally, and as long as they pass when run locally then that confirms that they pass. |
|
I tried the "dismiss review" button on Chris's previous "request changes" review, just in case that was related to why CI wasn't running. Doesn't seem like it had an effect — still not running. I don't even have a button to tell checks to run. Instead the "Checks" tab just looks like this:
It's as if there weren't any files under 🤷 We'll just rely on running checks locally, then. If this odd GitHub behavior starts happening repeatedly, we'll have to investigate further. |
Okay then. 
|
|
The GitHub UI is showing that there are some rebase conflicts: 
Could you please resolve those and comment here when that's done? If you need help, please ask in |
I can just resolve using the GitHub UI right? or would that add an unwanted commit? |
fombalang
force-pushed
the
autocomplete-results-list
branch
from
5b85e89 to
e3fb734
Compare
I have resolved the conflicts. I rebased my changes on top of the latest changes from main and fixed the conflicts. Please review when you can. |
Add `findAvatarImage` to check userId instead of URL, making it resilient to changes in avatar image details like size. Previous finder used 'findNetworkImage' which would fail if the avatar size is different Relevant Discussion: https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/ComposeAutocomplete.20test.20failure.20help zulip#995 (comment)
fombalang
force-pushed
the
autocomplete-results-list
branch
from
87d149c to
c584432
Compare
|
Hi all, and Happy New Year! 🎉 I've rebased my changes on top of the latest main, faced some difficulties cause they had diverged quite a bit but I was able to get it done. One thing to note, so a while ago we did the update about changing the implementation for the I did not want to touch that, so I added the new |
fombalang
force-pushed
the
autocomplete-results-list
branch
from
c584432 to
a31cb4a
Compare
|
Thanks @fombalang for the revision! Happy New Year.
That's fine — the emoji image URLs don't have the same tricky dependency on external facts (the device pixel ratio) that the avatar URLs do. Each image emoji has just a single size of image and a single URL to find it at. (Well, animated emoji have a second URL for the non-animated version; but the setting that controls whether to use that is a user setting entirely internal to the app, so is well controlled in tests.) So Looking at the rest of the changes now. |
There was a problem hiding this comment.
Thanks for the revision! Generally the implementation all looks good, with a couple of nits below. The remaining comments are mainly on the tests.
It looks like the commit I'd added to the tip of the branch earlier:
87d149c autocomplete: Update emoji-results style to follow design
got lost in the rebase. I'll push an updated version of it back to the PR branch. Before making your next changes, do be sure to fetch the updated version of the branch — git fetch me or git fetch origin, depending on the name you use for the Git remote pointing at your own GitHub fork of this repo. That way you'll have the version with my added commit and should naturally be able to carry it along as you revise.
test/widgets/autocomplete_test.dart
Outdated
| Finder findAvatarImage(int userId) => | ||
| find.byWidgetPredicate((widget) => widget is AvatarImage && widget.userId == userId); |
There was a problem hiding this comment.
nit:
| Finder findAvatarImage(int userId) => | |
| find.byWidgetPredicate((widget) => widget is AvatarImage && widget.userId == userId); | |
| Finder findAvatarImage(int userId) => | |
| find.byWidgetPredicate((widget) => widget is AvatarImage && widget.userId == userId); |
lib/widgets/autocomplete.dart
Outdated
| child: Row( | ||
| children: [ | ||
| avatar, |
There was a problem hiding this comment.
nit: preserve formatting of the parts you aren't otherwise changing:
| child: Row( | |
| children: [ | |
| avatar, | |
| child: Row(children: [ | |
| avatar, |
lib/widgets/autocomplete.dart
Outdated
| Expanded(child: Column( | ||
| mainAxisSize: MainAxisSize.min, |
There was a problem hiding this comment.
nit: two-space indents, here and elsewhere
| Expanded(child: Column( | |
| mainAxisSize: MainAxisSize.min, | |
| Expanded(child: Column( | |
| mainAxisSize: MainAxisSize.min, |
| checkUserShown(user1, store, expected: false); | ||
| checkUserShown(user2, store, expected: true); | ||
| checkUserShown(user3, store, expected: true); | ||
| checkUserShown(user1, expected: false); | ||
| checkUserShown(user2, expected: true); | ||
| checkUserShown(user3, expected: true); |
There was a problem hiding this comment.
The change where checkUserShown stops taking a store parameter should happen separately from the main "Implement new design" commit. That way it doesn't add more things happening inside that commit (which is the most complex commit in the branch).
In particular that way the reader doesn't have to wonder why the design change needs all these changes to the existing tests (which looks like what would happen if the design change were breaking something the existing tests were checking).
Probably cleanest is to have this change happen in the same commit that switches to findAvatarImage, because that's the change that makes the store unnecessary for this helper.
There was a problem hiding this comment.
For making that revision, the key tool is git rebase -i. The #git help channel is a good place to ask for advice.
The way I'd probably do it is:
git rebase -i- in the rebase TODO, put a
breakcommand before this commit - at that point, create a new commit that just makes this helper stop taking
store - let the rebase finish
- then in a second round of
git rebase -i, squash that new commit into the earlierfindAvatarImagecommit
For step 3, git checkout -p main is handy for taking pieces of the later commits in the branch. I'd probably use a mix of that together with just making the edits by hand in the IDE, for those parts where the desired change is mixed in with changes that should happen later. The command git diff -R main is handy for comparing your current tree to the tip of the branch, to see what changes will remain for the later commits in the branch.
test/widgets/autocomplete_test.dart
Outdated
| final user1 = eg.user(userId: 1, fullName: 'User One', avatarUrl: 'user1.png'); | ||
| final user2 = eg.user(userId: 2, fullName: 'User Two', avatarUrl: 'user2.png'); | ||
| final user3 = eg.user(userId: 3, fullName: 'User Three', avatarUrl: 'user3.png'); | ||
| final user1 = eg.user(userId: 1, fullName: 'User One', avatarUrl: 'user1.png',deliveryEmail: '[email protected]'); |
test/widgets/autocomplete_test.dart
Outdated
| }); | ||
|
|
||
| testWidgets('delivery email not visible when unavailable', (tester) async { | ||
| final user1 = eg.user(userId: 1, fullName: 'User One', avatarUrl: 'user1.png',); |
There was a problem hiding this comment.
Is the avatar URL needed? I think it shouldn't be relevant for this test — should be something the reader doesn't need to think about to understand the test. In that case we can keep this test case focused by leaving it out.
See also this description of how the data in a test should be "maximally boring":
https://chat.zulip.org/#narrow/channel/6-frontend/topic/typescript.20node.20tests/near/1896899
Similarly, is userId needed?
(OTOH fullName is relevant because it's what the test uses for finding the user in autocomplete.)
test/widgets/autocomplete_test.dart
Outdated
| // Options are filtered correctly for query | ||
| // TODO(#226): Remove this extra edit when this bug is fixed. | ||
| await tester.enterText(composeInputFinder, 'hello @user '); |
There was a problem hiding this comment.
| // Options are filtered correctly for query | |
| // TODO(#226): Remove this extra edit when this bug is fixed. | |
| await tester.enterText(composeInputFinder, 'hello @user '); | |
| // TODO(#226): Remove this extra edit when this bug is fixed. | |
| await tester.enterText(composeInputFinder, 'hello @user '); |
This test isn't checking anything about filtering. (The existing test above is checking filtering: "user t" matches "User Two" but not "User One".)
test/widgets/autocomplete_test.dart
Outdated
| TypingNotifier.debugEnable = false; | ||
| addTearDown(TypingNotifier.debugReset); |
There was a problem hiding this comment.
Huh, why do these two new tests need this and the existing test doesn't?
I suspect these don't need it either — it looks like setupToComposeInput handles it.
test/widgets/autocomplete_test.dart
Outdated
| debugNetworkImageHttpClientProvider = null; | ||
| }); | ||
| testWidgets('delivery email visible when available', (tester) async { | ||
| final user1 = eg.user(userId: 1, fullName: 'User One', avatarUrl: 'user1.png', deliveryEmail: '[email protected]'); |
There was a problem hiding this comment.
line too long — important information is past 80 columns
Concretely, here's what that line looks like for me right now:
As you can see, the delivery email is invisible.
There was a problem hiding this comment.
(I have a big monitor, but I use that horizontal space to show multiple things side by side, especially when reviewing. Often that's:
- the code, in my IDE
- another IDE pane with a related file, e.g. to read tests side by side with the code they're testing
- a terminal with
git log --stat -poutput, showing the content of the PR - a terminal with
git range-diffoutput, to compare the current revision of a PR with a previous one - the browser window with the GitHub page I'm typing the review into
)
test/widgets/autocomplete_test.dart
Outdated
| // Check "User One"'s delivery email is not visible | ||
| checkUserShown(user1, expected: true, deliveryEmailExpected: false); |
There was a problem hiding this comment.
This comment doesn't make a lot of sense, since User One has no delivery email in the first place 🙂
OTOH as I wrote at #995 (review) , this test case should check that user.email doesn't appear. It's easy to imagine having a bug where when there's no user.deliveryEmail, we show user.email instead — which would be wrong because user.email in that case is a meaningless fake email made up by the Zulip server.
There was a problem hiding this comment.
It may be cleanest to do that check by inlining the contents of checkUserShown into this test case (and the contrasting test case below), and adapting the details, rather than giving checkUserShown more parameters and conditions.
|
Hi @fombalang. Thanks for working on this! Do you think you will have time to continue working on this addressing the review comments? |
|
(The issue this addresses is an M6 Post-launch issue, but I'm still eager to get this PR finished and merged, as an exception to our stricter focus on launch issues — a lot of effort has gone into this already, and it's close to merge and will be great to have.) |
Add `findAvatarImage` to check userId instead of URL, making it resilient to changes in avatar image details like size. Previous finder used 'findNetworkImage' which would fail if the avatar size is different Relevant Discussion: https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/ComposeAutocomplete.20test.20failure.20help zulip#995 (comment)
|
In a call, we found that this PR is actually for an M5-a issue. I think the mislabeling is probably due to #913 (M5-a) and #914 (M6) being similar issues for different parts of the design. #913 is more feasible for completion near term because it only focuses on the redesign for autocomplete items. I will pick up this PR (reusing this branch) from here and address the comments. Thanks for all the work @fombalang, and the prior reviews from Chris and Greg! |
Add `findAvatarImage` to check userId instead of URL, making it resilient to changes in avatar image details like size. Previous finder used 'findNetworkImage' which would fail if the avatar size is different Relevant Discussion: https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/ComposeAutocomplete.20test.20failure.20help zulip#995 (comment)
PIG208
force-pushed
the
autocomplete-results-list
branch
from
e0d2988 to
6fc4065
Compare
|
The PR has been updated. It preserves most of the design changes, and on top of that:
Updated screenshot
|
PIG208
force-pushed
the
autocomplete-results-list
branch
from
6fc4065 to
c558e72
Compare
| /// 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? userDisplayEmail(User user) { |
There was a problem hiding this comment.
Logically this belongs on UserStore, but I guess it's in the same boat as hasPassedWaitingPeriod — we'll first need to arrange a good way for UserStore to access the realm settings.
There was a problem hiding this comment.
Perhaps like Unreads that rely on a reference to a ChannelStoreImpl, we can split out realm settings for UserStore. Probably a good refactor project as a follow-up.
There was a problem hiding this comment.
Yeah, or I'd organize it slightly differently to match the existing way UserStore works: see #1327 (comment) .
(Small correction: Unreads takes a ChannelStore, not a ChannelStoreImpl. Generally the only references to the "impl" classes are in PerAccountStore.)
| required BuildContext context, | ||
| required PerAccountStore store, |
There was a problem hiding this comment.
I wouldn't necessarily add this from scratch, but no need to cut it — it's an optimization, saving a duplicate PerAccountStoreWidget.of, and it's very cheap in terms of the code it requires.
lib/widgets/autocomplete.dart
Outdated
| avatar, | ||
| const SizedBox(width: 8), | ||
| label, | ||
| SizedBox.square(dimension: 36, child: avatar), |
There was a problem hiding this comment.
How does this work in the wildcard case, with the 24px icon? Do we need to do something to get the icon centered?
… Well, your screenshot in #995 (comment) seems to show it centered. I guess the Icon widget takes care of that internally.
There was a problem hiding this comment.
Anyway, in the user/avatar case, this SizedBox is redundant. Let's simplify a bit by moving it to within the wildcard case.
So effectively it becomes part of the avatar local's job to control its size, to avoid redundancy since when Avatar is used it will be controlling the size anyway.
There was a problem hiding this comment.
Yeah, from Icon.build:
return Semantics(
label: semanticLabel,
child: ExcludeSemantics(
child: SizedBox(width: iconSize, height: iconSize, child: Center(child: iconWidget)),
),
);There was a problem hiding this comment.
The SizedBox is ignored because the SizedBox in our code, and iconWidget is a RichText whose fontSize is iconSize.
test/widgets/autocomplete_test.dart
Outdated
| @@ -236,6 +233,67 @@ void main() { | |||
|
|
|||
| debugNetworkImageHttpClientProvider = null; | |||
| }); | |||
|
|
|||
| group('subLabel', () { | |||
There was a problem hiding this comment.
nit: match name to the one in the code (i.e. "sublabel")
| ).merge(weightVariableTextStyle(context, | ||
| wght: sublabel == null ? 500 : 600)), |
Moved method to PerAccountStore, renamed method from getDisplayEmailFor to userDisplayEmail and refactored usages to remove duplication.
Add `findAvatarImage` to check userId instead of URL, making it resilient to changes in avatar image details like size. Previous finder used 'findNetworkImage' which would fail if the avatar size is different Relevant Discussion: https://chat.zulip.org/#narrow/channel/516-mobile-dev-help/topic/ComposeAutocomplete.20test.20failure.20help zulip#995 (comment)
Signed-off-by: Zixuan James Li <[email protected]>
Signed-off-by: Zixuan James Li <[email protected]>
Implemented new design for @-mention autocomplete items. Added new `contextMenuItemLabel` and `contextMenuItemMeta` color variables to `designVariables` class. Fixes: zulip#913 Co-authored-by: Zixuan James Li <[email protected]> Signed-off-by: Zixuan James Li <[email protected]>
Fix some wrong color variables that were assigned to the contextMenuItemText field.
This follows the new design for user-mention results, but adapted for emoji based on the design for the emoji picker. See comment for details.
PIG208
force-pushed
the
autocomplete-results-list
branch
from
c558e72 to
2841a13
Compare
|
Thanks! This should be ready for review. |
|
Thanks! Looks good; merging. |
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
contextMenuItemLabelandcontextMenuItemMetacolor variables to thedesignVariablesclass 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.dartinitialSnapshotmethod where some of the arguments from theintialSnapshotmethod 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