-
Notifications
You must be signed in to change notification settings - Fork 373
message list: Add (you) indicator for self user in DM conversations #2019
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,9 @@ import '../generated/l10n/zulip_localizations.dart'; | |
| import '../model/narrow.dart'; | ||
| import '../model/recent_dm_conversations.dart'; | ||
| import '../model/unreads.dart'; | ||
| import '../model/store.dart'; | ||
|
|
||
| import 'color.dart'; | ||
| import 'icons.dart'; | ||
| import 'message_list.dart'; | ||
| import 'new_dm_sheet.dart'; | ||
|
|
@@ -169,26 +172,64 @@ class RecentDmConversationsItem extends StatelessWidget { | |
|
|
||
| static const double _avatarSize = 32; | ||
|
|
||
| TextSpan _buildUserTitleSpan( | ||
| BuildContext context, { | ||
| required int userId, | ||
| required PerAccountStore store, | ||
| }) { | ||
| final designVariables = DesignVariables.of(context); | ||
| final List<InlineSpan> spans = <InlineSpan>[]; | ||
|
|
||
| // Use store.userDisplayName so rendering rules remain consistent. | ||
| spans.add(TextSpan(text: store.userDisplayName(userId))); | ||
|
|
||
| // Status emoji inserted after name (keeps existing emoji rendering). | ||
| spans.add( | ||
| UserStatusEmoji.asWidgetSpan( | ||
| userId: userId, | ||
| fontSize: 17, | ||
| textScaler: MediaQuery.textScalerOf(context), | ||
| ), | ||
| ); | ||
|
Comment on lines
+191
to
+193
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still doesn't match the formatting of the existing code around it. Have you read the existing code yourself? Please take the time to do so. Having an AI tool look at the code is not a substitute. This particular instance is especially clear because this very diff is moving this code from elsewhere in the file, where it looks like so: UserStatusEmoji.asWidgetSpan(userId: store.selfUserId,
fontSize: 17, textScaler: MediaQuery.textScalerOf(context)), |
||
|
|
||
| // If this is the self user, add the localized "(you)" label. | ||
| if (userId == store.selfUserId) { | ||
| final youLabel = ZulipLocalizations.of(context).youLabel; | ||
| spans.add( | ||
| TextSpan( | ||
| text: ' $youLabel', | ||
| style: TextStyle( | ||
| color: designVariables.labelMenuButton.withFadedAlpha(0.5), | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| return TextSpan(children: spans); | ||
| } | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final store = PerAccountStoreWidget.of(context); | ||
| final designVariables = DesignVariables.of(context); | ||
|
|
||
| final InlineSpan title; | ||
| final Widget avatar; | ||
| int? userIdForPresence; | ||
|
|
||
| switch (narrow.otherRecipientIds) { // TODO dedupe with DM items in [InboxPage] | ||
| case []: | ||
| title = TextSpan(text: store.selfUser.fullName, children: [ | ||
| UserStatusEmoji.asWidgetSpan(userId: store.selfUserId, | ||
| fontSize: 17, textScaler: MediaQuery.textScalerOf(context)), | ||
| ]); | ||
| title = _buildUserTitleSpan( | ||
| context, | ||
| userId: store.selfUserId, | ||
|
Comment on lines
+221
to
+223
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this doesn't match the indentation style used in this project or any other. |
||
| store: store, | ||
| ); | ||
| avatar = AvatarImage(userId: store.selfUserId, size: _avatarSize); | ||
| case [var otherUserId]: | ||
| title = TextSpan(text: store.userDisplayName(otherUserId), children: [ | ||
| UserStatusEmoji.asWidgetSpan(userId: otherUserId, | ||
| fontSize: 17, textScaler: MediaQuery.textScalerOf(context)), | ||
| ]); | ||
| title = _buildUserTitleSpan( | ||
| context, | ||
| userId: otherUserId, | ||
| store: store, | ||
| ); | ||
| avatar = AvatarImage(userId: otherUserId, size: _avatarSize); | ||
| userIdForPresence = otherUserId; | ||
| default: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments don't make any sense for someone coming and reading the code in this file after this PR is merged. (What's the "existing" emoji rendering? What does it mean to "keep" it?) They therefore don't belong in the code.
They do make sense as (somewhat verbose) explanations of the diff in this PR.
Specifically, they read a lot like comments that an LLM coding tool might insert, when asked to write a change like this, to explain the diff to you.
That sort of comment isn't helpful in a PR. When you send a PR for us to review, you need to already understand it yourself. Then you need to reread it to make sure it expresses your thoughts clearly and concisely. See our AI policy:
https://zulip.readthedocs.io/en/latest/contributing/contributing.html#ai-use-policy-and-guidelines
(When there is something to say about the code diff, rather than the resulting code, the home for that is the commit messages rather than in-code comments. See https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#commit-messages-code . But the comments here are too redundant to be necessary at all.)