-
Notifications
You must be signed in to change notification settings - Fork 407
Add DM action sheet, with "View profile" and "Mark conversation as read" buttons #2114
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
bb69505 to
40f6fe9
Compare
|
Updated for Alya's feedback at #mobile > abbreviated headings @ 💬:
Also see Greg's messages, from #mobile > abbreviated headings @ 💬:
So, marking as ready for review. |
40f6fe9 to
672f09f
Compare
|
(fixed an unused-import nit flagged by the analyzer) |
rajveermalviya
left a comment
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.
Thanks @chrisbobbe! All LGTM, modulo one tiny nit.
Moving over to Greg's review.
lib/widgets/message_list.dart
Outdated
| ]); | ||
|
|
||
| case DmNarrow(:var otherRecipientIds): | ||
| case DmNarrow(:var otherRecipientIds): { |
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.
nit: unnecessary braces?
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.
Sure; removed for my next revision.
672f09f to
d766e61
Compare
|
Thanks! Revision pushed. |
gnprice
left a comment
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.
Thanks! Generally this looks great; small comments below.
lib/widgets/action_sheet.dart
Outdated
|
|
||
| [ | ||
| if (narrow.otherRecipientIds.isEmpty) | ||
| ViewProfileButton(pageContext: context, |
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.
| ViewProfileButton(pageContext: context, | |
| ViewProfileButton(pageContext: pageContext, |
?
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 yeah, and I notice a few existing places we made this same mistake; I'll add a commit to fix those.
lib/widgets/action_sheet.dart
Outdated
| ]; | ||
|
|
||
| final header = BottomSheetHeader( | ||
| title: switch (narrow) { |
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.
nit: call the otherRecipientIds getter once here, instead of once per case, so as to build the list just once
| title: switch (narrow) { | |
| title: switch (narrow.otherRecipientIds) { |
In fact since there's a few other references above, probably best to pull it out as a local.
test/widgets/action_sheet_test.dart
Outdated
| group('smoke', () { | ||
| group('from inbox', () { |
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.
nit: collapse the singleton group; and can be a bit more specific about what these tests cover:
| group('smoke', () { | |
| group('from inbox', () { | |
| group('show from inbox', () { |
Namely, they cover causing the action sheet to show and how it looks when shown, while the behavior of the buttons is left to other tests below.
test/widgets/action_sheet_test.dart
Outdated
| ]); | ||
| await tester.tap(find.widgetWithText(UserChip, eg.otherUser.fullName)); |
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 maybe deserves highlighting as a separate test case. But at a minimum give it a separate stanza and a comment line:
| ]); | |
| await tester.tap(find.widgetWithText(UserChip, eg.otherUser.fullName)); | |
| ]); | |
| // Tapping a user in the header leads to their profile. | |
| await tester.tap(find.widgetWithText(UserChip, eg.otherUser.fullName)); |
test/widgets/action_sheet_test.dart
Outdated
| check(findInHeader(find.text('DMs with ${eg.otherUser.fullName}'))) | ||
| .findsOne(); | ||
| check(store.unreads.countInDmNarrow(narrow)).equals(0); | ||
| check(findInHeader(find.text('DMs with ${eg.otherUser.fullName}'))) | ||
| .findsOne(); |
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 two findInHeader checks are redundant, right?
… weren't Topic-row UI is naturally ephemeral; it could disappear if the topic ceases to exist, or you read all its messages (as in the Inbox), etc., which is why we prefer not to use its BuildContext where we could use the whole page's context instead.
Fixes zulip#1272. While working on the design update for the Inbox page, I noticed that we were well-prepared to implement the DM action sheet, including a way to show the participants of a group DM in the action sheet itself. For that, I've used a scrollable collection of user "chips" in the action-sheet header, with avatar, presence, and name. Discussion ongoing for whether we like this approach or prefer to put the users list on a separate page, as in issue zulip#1534: https://chat.zulip.org/#narrow/channel/48-mobile/topic/abbreviated.20headings/near/2366774 In this commit, the action sheet can have up to two buttons: - "View profile", for 1:1 DMs - "Mark conversation as read", if there are unreads.
d766e61 to
eabc496
Compare
|
Thanks for the review! Revision pushed. |


Fixes #1272.
(This is a draft because there's an open discussion; see the "scrollable when long" screenshots and the CZO link there.)The action sheet is offered in all four places mentioned in the issue:
Self-DM:
1:1 DM:
Group DM:
List of users in group DM, which is scrollable when long. We might still prefer a different page for this list (#1534); see #mobile > abbreviated headings @ 💬: