-
Notifications
You must be signed in to change notification settings - Fork 310
Distinguish muted users in UI #1429
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?
Conversation
c34b94b
to
d80442b
Compare
86d0e66
to
db3216f
Compare
db3216f
to
17f2f6b
Compare
8ab889f
to
cddcfcc
Compare
@chrisbobbe This is now ready for a general review, as we discussed on the call this week. PTAL. I am now starting to write tests. |
@@ -0,0 +1,3 @@ | |||
<svg width="18" height="20" viewBox="0 0 18 20" fill="none" xmlns="http://www.w3.org/2000/svg"> | |||
<path fill-rule="evenodd" clip-rule="evenodd" d="M9 1.9975C6.78948 1.9975 4.9975 3.78948 4.9975 6C4.9975 8.21052 6.78948 10.0025 9 10.0025C11.2105 10.0025 13.0025 8.21052 13.0025 6C13.0025 3.78948 11.2105 1.9975 9 1.9975ZM12.6361 10.77C14.0714 9.67415 14.9975 7.94523 14.9975 6C14.9975 2.68767 12.3123 0.00250244 9 0.00250244C5.68767 0.00250244 3.0025 2.68767 3.0025 6C3.0025 7.94523 3.92857 9.67415 5.36389 10.77C4.3572 11.2147 3.43107 11.8445 2.63781 12.6378C0.950451 14.3252 0.00250244 16.6137 0.00250244 19C0.00250244 19.5509 0.449098 19.9975 1 19.9975C1.55091 19.9975 1.9975 19.5509 1.9975 19C1.9975 17.1428 2.73526 15.3617 4.04849 14.0485C5.36171 12.7353 7.14282 11.9975 9 11.9975C10.8572 11.9975 12.6383 12.7353 13.9515 14.0485C15.2647 15.3617 16.0025 17.1428 16.0025 19C16.0025 19.5509 16.4491 19.9975 17 19.9975C17.5509 19.9975 17.9975 19.5509 17.9975 19C17.9975 16.6137 17.0496 14.3252 15.3622 12.6378C14.5689 11.8445 13.6428 11.2147 12.6361 10.77Z" fill="black"/> | |||
</svg> |
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.
This icon file is displayed differently when used as ZulipIcon. In the file, the head section is an outlined circle, but in the app, it is a filled circle.
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.
Yeah, fill-rule="evenodd"
doesn't work when converted to a font. Here's instructions for fixing it:
https://zulip.readthedocs.io/en/latest/subsystems/icons.html#correcting-icons-with-an-evenodd-fill-rule
cddcfcc
to
3df45ad
Compare
Thanks, glad to see this progress on this helpful feature! Comments on the first four commits: 451ec5a api: Add InitialSnapshot.mutedUsers Let's put the API-binding changes in their own case MutedUsersEvent():
// TODO handle but that's easy to do. 🙂 Then please send those revised commits as a new, non-draft PR and label it for Greg's review:
Thanks! I'll start reading the later commits now. |
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.
And here's a review of the next commit 🙂:
7072228 msglist: Distinguish messages sent by muted users
class PossibleMutedMessage extends InheritedWidget { | ||
const PossibleMutedMessage({ | ||
super.key, | ||
required this.changeMuteStatus, | ||
required super.child, | ||
}); | ||
|
||
final ValueChanged<bool> changeMuteStatus; | ||
|
||
@override | ||
bool updateShouldNotify(covariant PossibleMutedMessage oldWidget) => false; | ||
|
||
static PossibleMutedMessage of(BuildContext context) { | ||
final value = context.getInheritedWidgetOfExactType<PossibleMutedMessage>(); | ||
assert(value != null, 'No PossibleMutedMessage ancestor'); | ||
return value!; | ||
} | ||
} |
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.
Interesting; the ValueChanged
typedef was new to me!
I think a different approach would be worthwhile, though, toward more familiar/transparent state management and to minimize added code in _MessageListPageState.build
.
-
On
MessageListPageState
, add methods like the following:/// The "revealed" state of a message from a muted sender. /// /// See also: [revealMutedMessage], [unrevealMutedMessage]. bool isMutedMessageRevealed(int messageId); /// For a message from a muted sender, reveal the sender and content, /// replacing the "Muted sender" placeholder. void revealMutedMessage(int messageId); /// For a message from a muted sender, hide the sender and content again /// with the "Muted sender" placeholder. void unrevealMutedMessage(int messageId);
-
Implement these in
_MessageListPageState
, backed by aSet
of message IDs. The implementations should be simple and don't need to involve the store or any event handling; just simple add/remove/read. -
Read the dartdoc on
MessageListPage.ancestorOf
and the implementation comment under it. Since we'll need to call this from a build method, make a prep commit that creates anInheritedWidget
, so thatancestorOf
can become something like:/// The [MessageListPageState] above this context in the tree. /// /// Uses the efficient [BuildContext.dependOnInheritedWidgetOfExactType], /// so this may be called in a build method. static MessageListPageState ancestorOf(BuildContext context) { final state = context .dependOnInheritedWidgetOfExactType<_MessageListPageInheritedWidget>() .state; assert(state != null, 'No _MessageListPageInheritedWidget ancestor'); return state!; }
Ah—in fact I think it might need to be an
InheritedNotifier
, with thereveal…
/unreveal…
methods callingnotifyListeners
, so that widgets update whenisMutedMessageRevealed
has a new value? -
HideMutedMessageButton
(perhaps renamedUnrevealMutedMessageButton
for consistency) can just do this in itsonPressed
:MessageListPage.ancestorOf(pageContext).unrevealMutedMessage(message.id);
-
The "Reveal" button in the message list can do this:
MessageListPage.ancestorOf(context).revealMutedMessage(message.id);
-
And the UI state in
_MessageWithPossibleSenderState.build
can useMessageListPage.ancestorOf(context).isMutedMessageRevealed
.
"revealButtonLabel": "Reveal message for muted sender", | ||
"@revealButtonLabel": { | ||
"description": "Label for the button revealing hidden message from a muted sender in message list." | ||
}, |
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.
This feels verbose; how about just "Reveal message"? The sender row already says "Muted sender".
"mutedSender": "Muted sender", | ||
"@mutedSender": { | ||
"description": "Name for a muted user to display in message list." | ||
}, |
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.
How about "Muted user"? The fact that it's the sender is already communicated by the position of this text.
@@ -128,6 +128,10 @@ | |||
"@actionSheetOptionMarkAsUnread": { | |||
"description": "Label for mark as unread button on action sheet." | |||
}, | |||
"actionSheetOptionHideMutedMessage": "Hide muted message again", |
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.
msglist: Distinguish messages sent by muted users
Commit-message nit: no Fixes:
line until a later commit, when all of the issue's work is done :)
class AvatarPlaceholder extends StatelessWidget { | ||
const AvatarPlaceholder({super.key, this.size}); | ||
|
||
final double? size; |
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.
If all callers pass size
, let's make it required
// Scale the icon proportionally to match the Figma design. | ||
size: size != null ? size! * 20 / 32 : null, |
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.
What's the meaning of the size
param, and why isn't it used directly here? When I read this I wonder why callers don't just pass the size they want (e.g. from a Figma frame) and expect it to be applied faithfully :)
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.
Ah I think I see: size
is really about the dimensions of the square gray box, and the icon is meant to be some fraction of that, right?
Oh, but a nuance: the actual size of the box is controlled by AvatarShape
, which callers are supposed to use as a wrapper. So I think the size
param calls for a dartdoc explaining why it's needed; something like:
/// The size of the placeholder box.
///
/// This should match the `size` passed to the wrapping [AvatarShape].
/// The placeholder's "person" icon will be scaled proportionally to this.
Then here, on the arithmetic, an implementation comment like:
// Where the avatar placeholder appears in the Figma,
// this is how the icon is sized proportionally to its box.
required this.grey250, | ||
required this.grey550, |
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.
Ah, these aren't Figma design variables in the usual way—

—they're part of a "color palette" (see #831).
Since the Figma doesn't seem to offer a variable, let's first see how these look in dark mode (please post screenshots); if they look reasonable (even if not perfect), I'd just hard-code the colors inline, with a TODO(design): dark-mode variant?
. If we need a dark-mode variant, please @-mention Vlad in #mobile.
Widget? revealButton; | ||
if (showAsMuted) { | ||
revealButton = TextButton.icon( | ||
onPressed: () { | ||
changeMuteStatus(false); | ||
}, | ||
style: TextButton.styleFrom( | ||
minimumSize: Size.zero, | ||
padding: EdgeInsets.symmetric(vertical: 4, horizontal: 6), | ||
tapTargetSize: MaterialTapTargetSize.shrinkWrap, | ||
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(6)), | ||
splashFactory: NoSplash.splashFactory, | ||
).copyWith( | ||
backgroundColor: WidgetStateColor.fromMap({ | ||
WidgetState.pressed: designVariables.neutralButtonBg, | ||
~WidgetState.pressed: designVariables.neutralButtonBg | ||
.withFadedAlpha(0), | ||
}), | ||
foregroundColor: WidgetStateColor.fromMap({ | ||
WidgetState.pressed: designVariables.neutralButtonLabel, | ||
~WidgetState.pressed: designVariables.neutralButtonLabel | ||
.withFadedAlpha(0.85), | ||
}), | ||
), | ||
icon: Icon(ZulipIcons.eye), | ||
label: Text(zulipLocalizations.revealButtonLabel, | ||
style: TextStyle(fontSize: 16, height: 1) | ||
.merge(weightVariableTextStyle(context, wght: 600)))); | ||
} |
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.
With 20+ lines of code, this would be good to pull out into a separate widget or a helper method
Also renamed "user" to "two_person" to make it consistent with other icons of the same purpose. New icons taken from: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5968-237884&t=dku3J5Fv2dmWo7ht-0 https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5968-264632&t=dku3J5Fv2dmWo7ht-0 https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5968-291260&t=B5jAmeUMMG4dlsU7-0
The conversations are excluded where all the recipients are muted.
This is solely for a better order.
Direct messages of those conversations are excluded where all the recipients are muted. Currently the applicable narrows are: - CombinedFeedNarrow - MentionsNarrow - StarredMessagesNarrow In the future, this will apply to the ReactionsNarrow from the Web app too, once we have it.
3df45ad
to
c1a1982
Compare
"Reveal message for muted sender" is an odd phrase. I think @terpimost 's design just had "Reveal": ![]() Was this changed because we decided to put the button on a separate line from the sender? Even so, maybe "Reveal message" is best -- I think it'll be clear enough. |
Note: No tests are included in this draft revision.
This PR aims to respect the user's settings for muting other users by distinguishing its related UI appearance. From the list of places in UI mentioned in https://zulip.com/help/mute-a-user, the following are included as they're compatible with the current scope of mobile:
composing a direct message ormentioning a user.Fixes: #296