message: Improve muted sender design.#34794
Conversation
27da778 to
19804a2
Compare
|
@whilstsomebody, can you do a buddy review if you find time? |
19804a2 to
48c9f1a
Compare
|
Tag me again when this is ready for a review. |
48c9f1a to
eaa498b
Compare
|
tagging @alya, @terpimost for a design review |
6a8d7a0 to
b38fc49
Compare
|
How about "Muted user" in place of "Muted sender"? That's our plan in mobile; the fact that it's the sender is already communicated by the position of the text. (Thanks @sm-sayedi for mentioning this at zulip/zulip-flutter#1429 (comment) ) |
b38fc49 to
36b317f
Compare
36b317f to
a17f0de
Compare
|
tagging @whilstsomebody for buddy review. |
whilstsomebody
left a comment
There was a problem hiding this comment.
Left some comments.
Looks good in the manual testiing.
| @@ -1,5 +1,5 @@ | |||
| <div id="message-row-{{message_list_id}}-{{msg/id}}" data-message-id="{{msg/id}}" | |||
| class="message_row{{#unless msg/is_stream}} private-message{{/unless}}{{#if include_sender}} messagebox-includes-sender{{/if}}{{#if mention_classname}} {{mention_classname}}{{/if}}{{#if msg.unread}} unread{{/if}} {{#if msg.locally_echoed}}locally-echoed{{/if}} selectable_row" | |||
| class="message_row{{#unless msg/is_stream}} private-message{{/unless}}{{#if include_sender}} messagebox-includes-sender{{/if}}{{#if mention_classname}} {{mention_classname}}{{/if}}{{#if msg.unread}} unread{{/if}} {{#if msg.locally_echoed}}locally-echoed{{/if}} selectable_row {{#if is_hidden}}muted_message_sender{{/if}}" | |||
There was a problem hiding this comment.
muted-message-sender and not muted_message_sender.
9dcd249 to
f0968e3
Compare
whilstsomebody
left a comment
There was a problem hiding this comment.
Looks good to me.
|
@zulipbot add "mentor review" |
|
Thanks @whilstsomebody! |
fed6773 to
c03325c
Compare
For this, I introduced a
previous version:
|
8c7f8da to
4f9ae38
Compare
|
The margin-left approach is probably fine for now. |
|
@karlstolley Any more feedback from your side before this goes to integration review? |
|
Left a question about what looks now like some dead CSS. Commit history also needs to be cleaned up and finalized before we can mark this one for integration review. |
4f9ae38 to
72418d3
Compare
|
Addressed the comment and updated the commit history. |
72418d3 to
8a1d55c
Compare
|
The DM part can be a follow-up if it's not a regression in this PR. |
|
This is how it looks like on my end, I think the pills have to be updated to show the Muted user name and avatar. Probably worth addressing as a follow-up? Edit: But wait, what if there are multiple users in these pills, say for a group DM and all are muted, I don't think having multiple pills showing "Muted user" makes sense in that case. I imagine this will also require performing multiple clicks to show/hide the users behind these muted user pills, so not sure how useful this pill masking feature might be. Maybe knowing who the DM participants are but with hidden messages (so you know you muted them) is better UX wise? |
|
Cool. Yes, let's address the DM issue as a prompt follow up. @apoorvapendse, probably worth opening a #design discussion about the group DMs case, if a search of CZO doesn't turn up previous discussions of that. |
|
@karlstolley I think the PR is ready for integration review, then? |
|
Opened #design > muted user UX for DMs for the follow-up discussion. |
| @@ -1,6 +1,6 @@ | |||
| <div class="u-{{msg/sender_id}} message-avatar sender_info_hover view_user_card_tooltip no-select" aria-hidden="true" data-is-bot="{{sender_is_bot}}"> | |||
| <div class="u-{{msg/sender_id}} message-avatar {{#unless is_hidden}}sender_info_hover view_user_card_tooltip{{/unless}} no-select" aria-hidden="true" data-is-bot="{{sender_is_bot}}"> | |||
There was a problem hiding this comment.
Why is the tooltip for viewing the user card being disabled here? I feel like it's totally reasonable to want that tooltip to work for muted users, if we're going to show an avatar element.
There was a problem hiding this comment.
Just removing this conditional seems to not make that work. I think we can change this in a follow-up; added it to the list at the top of the PR.
| <i role="button" tabindex="0" class="message-controls-icon star zulip-icon {{#if msg/starred}}zulip-icon-star-filled{{else}}zulip-icon-star{{/if}}"></i> | ||
| {{/unless}} | ||
| </div> | ||
|
|
There was a problem hiding this comment.
Let's not add a blank line here in this commit.
There was a problem hiding this comment.
Fixed this myself so we can merge this.
If the message needs to be hidden because the sender was muted, we replace the sender's avatar with that of a muted sender and name them as "Muted sender". Signed-off-by: apoorvapendse <apoorvavpendse@gmail.com>
Signed-off-by: apoorvapendse <apoorvavpendse@gmail.com>
This is reworded because we will introduce a reveal button instead of the "Click here to reveal" link. Signed-off-by: apoorvapendse <apoorvavpendse@gmail.com>
We want to avoid showing the hidden message notice on mobile widths. Discussion: https://chat.zulip.org/#narrow/channel/101-design/topic/muted.20users/near/2189225 Signed-off-by: apoorvapendse <apoorvavpendse@gmail.com>
We keep the reveal button invisible until hovered. This mechanism is only followed for devices where hover is possible, on touchscreens the "Reveal" button is visible by default. Signed-off-by: apoorvapendse <apoorvavpendse@gmail.com>
Fixes: zulip#32945. Signed-off-by: apoorvapendse <apoorvavpendse@gmail.com>
8a1d55c to
290524c
Compare
|
Merged with the changes detailed in #34794 (comment), thanks for all the work on this @apoorvapendse and @karlstolley! @apoorvapendse please consider the two follow-ups to be priorities; it'd be nice if those made it to Zulip Cloud at the same time as this change. |










Fixes: #32945.
If the message needs to be hidden because the sender was muted,
we replace the sender's avatar with that of a muted sender
and name them as "Muted sender".
This also introduces a new reveal button to show the message.
We hide the muted message notice on mobile widths.
Discussion: https://chat.zulip.org/#narrow/channel/101-design/topic/muted.20users/near/2185476
Fixes: #32945.
CZO: #design > muted users
Follow-up items
- [ ] Figure out pills for muted participants in DMs and group DMs czoUpdated version can be found at #34794 (comment)
Screenshots and screen captures:
I am doing this with
cq_sm_minas the breakpoint for now.I hid the
reveal labelmuted sender message notice at thecq_mm_minbreakpoint to avoid truncating "Muted user" on small screens and reduce visual clutter.Feedback welcome.
Device: iPhone 12 Pro (390x844)
Self-review checklist