-
Notifications
You must be signed in to change notification settings - Fork 270
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
action_sheet: Offer "Mark channel as read" in channel action sheet #1317
base: main
Are you sure you want to change the base?
Conversation
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! Comments below.
0a8811a
to
8a4937e
Compare
@chrisbobbe I’ve addressed all the requested changes. Please take a look. Thank you! |
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 for the revision! Comments below.
lib/widgets/inbox.dart
Outdated
@override | ||
Future<void> onLongPress() async { | ||
// TODO(#1272) action sheet for DM conversation | ||
return; | ||
} |
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.
bump on #1317 (comment) ; let me know if it doesn't make sense.
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.
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.
I see; trying this myself, this is a little tricky. Try this change (squashed into the same commit):
diff --git lib/widgets/inbox.dart lib/widgets/inbox.dart
index 551c14bdd..8c8217b6e 100644
--- lib/widgets/inbox.dart
+++ lib/widgets/inbox.dart
@@ -255,7 +255,6 @@ abstract class _HeaderItem extends StatelessWidget {
}
Future<void> onRowTap();
- Future<void> onLongPress();
@override
Widget build(BuildContext context) {
@@ -273,7 +272,9 @@ abstract class _HeaderItem extends StatelessWidget {
// But that's in tension with the Figma, which gives these header rows
// 40px min height.
onTap: onCollapseButtonTap,
- onLongPress: onLongPress,
+ onLongPress: this is _LongPressable
+ ? (this as _LongPressable).onLongPress
+ : null,
child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [
Padding(padding: const EdgeInsets.all(10),
child: Icon(size: 20, color: designVariables.sectionCollapseIcon,
@@ -332,12 +333,6 @@ class _AllDmsHeaderItem extends _HeaderItem {
pageState.allDmsCollapsed = !collapsed;
}
@override Future<void> onRowTap() => onCollapseButtonTap(); // TODO open all-DMs narrow?
-
- @override
- Future<void> onLongPress() async {
- // TODO(#1272) action sheet for DM conversation
- return;
- }
}
class _AllDmsSection extends StatelessWidget {
@@ -439,7 +434,13 @@ class _DmItem extends StatelessWidget {
}
}
-class _StreamHeaderItem extends _HeaderItem {
+mixin _LongPressable on _HeaderItem {
+ // TODO(#1272) move to _HeaderItem base class
+ // when DM headers become long-pressable; remove mixin
+ Future<void> onLongPress();
+}
+
+class _StreamHeaderItem extends _HeaderItem with _LongPressable {
final Subscription subscription;
const _StreamHeaderItem({
06ed689
to
93d3199
Compare
5768d19
to
da7af54
Compare
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, this is getting closer!
One tip that would be helpful for me as I review: when you use GitHub's feature for marking review comments as resolved, it makes it harder for me to go back through each one and check the new revision against it, because I have to click "show resolved" on each one. When I'm responding to a review, I like to just put a 👍 on a comment after I've addressed it, which helps me keep track without causing that problem. 🙂
Also see my reply to a question above: #1317 (comment)
lib/widgets/actions.dart
Outdated
Future<void> unregisterToken(GlobalStore globalStore, int accountId) async { | ||
Future<void> unregisterToken(GlobalStore globalStore, int accountId) async { |
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: remove unwanted spacing change
lib/widgets/actions.dart
Outdated
} | ||
} | ||
|
||
final didPass = await updateMessageFlagsStartingFromAnchor( |
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.
updateMessageFlagsStartingFromAnchor
also gives UI feedback, so it also belongs in the new ZulipAction
class.
lib/widgets/inbox.dart
Outdated
@override | ||
Future<void> onLongPress() async { | ||
// TODO(#1272) action sheet for DM conversation | ||
return; | ||
} |
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.
I see; trying this myself, this is a little tricky. Try this change (squashed into the same commit):
diff --git lib/widgets/inbox.dart lib/widgets/inbox.dart
index 551c14bdd..8c8217b6e 100644
--- lib/widgets/inbox.dart
+++ lib/widgets/inbox.dart
@@ -255,7 +255,6 @@ abstract class _HeaderItem extends StatelessWidget {
}
Future<void> onRowTap();
- Future<void> onLongPress();
@override
Widget build(BuildContext context) {
@@ -273,7 +272,9 @@ abstract class _HeaderItem extends StatelessWidget {
// But that's in tension with the Figma, which gives these header rows
// 40px min height.
onTap: onCollapseButtonTap,
- onLongPress: onLongPress,
+ onLongPress: this is _LongPressable
+ ? (this as _LongPressable).onLongPress
+ : null,
child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [
Padding(padding: const EdgeInsets.all(10),
child: Icon(size: 20, color: designVariables.sectionCollapseIcon,
@@ -332,12 +333,6 @@ class _AllDmsHeaderItem extends _HeaderItem {
pageState.allDmsCollapsed = !collapsed;
}
@override Future<void> onRowTap() => onCollapseButtonTap(); // TODO open all-DMs narrow?
-
- @override
- Future<void> onLongPress() async {
- // TODO(#1272) action sheet for DM conversation
- return;
- }
}
class _AllDmsSection extends StatelessWidget {
@@ -439,7 +434,13 @@ class _DmItem extends StatelessWidget {
}
}
-class _StreamHeaderItem extends _HeaderItem {
+mixin _LongPressable on _HeaderItem {
+ // TODO(#1272) move to _HeaderItem base class
+ // when DM headers become long-pressable; remove mixin
+ Future<void> onLongPress();
+}
+
+class _StreamHeaderItem extends _HeaderItem with _LongPressable {
final Subscription subscription;
const _StreamHeaderItem({
@override | ||
void onPressed() async { | ||
final narrow = ChannelNarrow(streamId); | ||
await ZulipAction.markNarrowAsRead(pageContext, 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.
I see; the reasoning for ZulipAction
, in its dartdoc, makes sense.
I see that ZulipAction.markNarrowAsRead
takes care of giving UI feedback, so I agree a try
/ catch
isn't needed here.
It still doesn't give one specific kind of feedback I was hoping for, though 🙂:
Let's have special handling for if e is ZulipApiException, like other buttons that make API requests
Could you make that adjustment, in a separate commit, for both values of useLegacy
? For useLegacy
false
, that means adjusting updateMessageFlagsStartingFromAnchor
, which is where the UI-feedback code is in that case.
test/widgets/action_sheet_test.dart
Outdated
Future<void> prepare({UnreadMessagesSnapshot? unreadMsgs}) async { | ||
final stream = eg.stream(); | ||
someChannel = stream; | ||
addTearDown(testBinding.reset); | ||
|
||
unreadMsgs ??= eg.unreadMsgs(channels: [ | ||
eg.unreadChannelMsgs( | ||
streamId: stream.streamId, | ||
topic: 'topic', | ||
unreadMessageIds: [1], | ||
), | ||
]); | ||
|
||
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot( | ||
realmUsers: [eg.selfUser, eg.otherUser], | ||
streams: [someChannel], | ||
subscriptions: [eg.subscription(someChannel)], | ||
unreadMsgs: unreadMsgs)); | ||
store = await testBinding.globalStore.perAccount(eg.selfAccount.id); | ||
connection = store.connection as FakeApiConnection; | ||
} |
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.
Please read through the indentation in this file and make it consistent with the project style; this part (and at least one other part) is currently hard to read.
test/widgets/action_sheet_test.dart
Outdated
Future<void> showFromInbox(WidgetTester tester) async { | ||
await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, | ||
child: const HomePage())); | ||
await tester.pumpAndSettle(); |
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.
Can we replace this with await tester.pump();
and remove the await tester.pump();
below? The 'topic action sheet' tests are doing fine that way.
test/widgets/action_sheet_test.dart
Outdated
await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, | ||
child: const SubscriptionListPageBody())); | ||
await tester.pumpAndSettle(); | ||
check(find.byType(SubscriptionListPageBody)).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.
await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, | |
child: const SubscriptionListPageBody())); | |
await tester.pumpAndSettle(); | |
check(find.byType(SubscriptionListPageBody)).findsOne(); | |
await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, | |
child: const HomePage())); | |
await tester.pump(); | |
await tester.tap(find.byIcon(ZulipIcons.hash_italic)); | |
await tester.pump(); | |
check(find.byType(SubscriptionListPageBody)).findsOne(); |
To be more representative of the real app.
test/widgets/action_sheet_test.dart
Outdated
await tester.pumpAndSettle(); | ||
check(find.byType(SubscriptionListPageBody)).findsOne(); | ||
|
||
await tester.pump(); |
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.
(I think this pump is unnecessary, as in showFromInbox
.)
test/widgets/action_sheet_test.dart
Outdated
connection.prepare(httpStatus: 400, json: { | ||
'result': 'error', 'code': 'BAD_REQUEST', 'msg': ''}); |
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, like in the topic action sheet test:
connection.prepare(exception: http.ClientException('Oops'));
which is shorter.
test/widgets/action_sheet_test.dart
Outdated
await tester.pump(); // | ||
await tester.pumpAndSettle(); // Wait for dialog animation |
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.
The await tester.pump(); //
looks unnecessary, comparing with the corresponding topic action sheet test.
9b825ee
to
2a5b01d
Compare
Thanks for the review @chrisbobbe .Pushed a new revision, PTAL. I'm a bit unsure if my revision on #1317 (comment) is correct. |
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 for the revision!
When trying out the UI on an iPhone, I noticed some more places that we might want to offer this action sheet from, and I started a CZO discussion: https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/channel.20action.20sheet/near/2092028
I believe your next revision (for my comments below) doesn't need to be blocked on waiting for a conclusion there.
lib/widgets/actions.dart
Outdated
@@ -20,6 +20,162 @@ import '../notifications/receive.dart'; | |||
import 'dialog.dart'; | |||
import 'store.dart'; | |||
|
|||
/// High-level operations that combine API calls with UI feedback. |
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.
actions [nfc]: Introduce ZulipAction for markNarrowAsRead & updateMessageFlagsStartingFromAnchor
commit-message nit: summary line looks too long (longer than 76 columns), and body text should be wrapped to 68 (you can configure your text editor to help with this).
lib/widgets/actions.dart
Outdated
await updateMessageFlagsStartingFromAnchor( | ||
await ZulipAction.updateMessageFlagsStartingFromAnchor( |
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: since markNarrowAsUnreadFromMessage
calls a ZulipAction
method, that means it also gives UI feedback and should move to ZulipAction
too.
lib/widgets/action_sheet.dart
Outdated
void showChannelActionSheet(BuildContext context, { | ||
required int streamId, | ||
}) { |
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.
One useful change was merged after you started working on this PR 🙂: please see the top of showTopicActionSheet
in main
, and use PageRoot.contextOf(context)
the way we do there.
lib/widgets/action_sheet.dart
Outdated
@@ -163,6 +163,58 @@ class ActionSheetCancelButton extends StatelessWidget { | |||
} | |||
} | |||
|
|||
/// Show a sheet of actions you can take on a channel. | |||
void showChannelActionSheet(BuildContext context, { | |||
required int streamId, |
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.
Let's use the modern term channelId
instead of streamId
. (There are still several places in the existing code that haven't migrated yet; we'll address those separately.)
checkErrorDialog(tester, | ||
expectedTitle: "Mark as read failed"); |
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.
Might as well also do a checkRequest
here too.
2a5b01d
to
e5af07a
Compare
@chrisbobbe Pushed the revision. Please take a look. Also implemented opening the channel action sheet from the places mentioned in the CZO discussion. |
I notice that the touch target in the app bar has gotten much smaller, which will be particularly problematic for short topic or stream names. Screenshots from an iPhone simulator, using "Select widget mode" in the "Flutter inspector" tab of the Flutter dev tools:
Let's have the
To do that, I suggest:
So like this: case TopicNarrow(:var streamId, :var topic):
final store = PerAccountStoreWidget.of(context);
final stream = store.streams[streamId];
final alignment = willCenterTitle
? Alignment.center
: AlignmentDirectional.centerStart;
return Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
children: [
GestureDetector(
behavior: HitTestBehavior.translucent,
onLongPress: () {
showChannelActionSheet(context, channelId: streamId);
},
child: Align(alignment: alignment,
child: _buildStreamRow(context, stream: stream)),
),
GestureDetector(
behavior: HitTestBehavior.translucent,
onLongPress: () {
final someMessage = MessageListPage.ancestorOf(context)
.model?.messages.firstOrNull;
// If someMessage is null, the topic action sheet won't have a
// resolve/unresolve button. That seems OK; in that case we're
// either still fetching messages (and the user can reopen the
// sheet after that finishes) or there aren't any messages to
// act on anyway.
assert(someMessage == null || narrow.containsMessage(someMessage));
showTopicActionSheet(context,
channelId: streamId,
topic: topic,
someMessageIdInTopic: someMessage?.id);
},
child: Align(alignment: alignment,
child: _buildTopicRow(context, stream: stream, topic: topic)))]); |
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! Here's the rest of a review of this revision, and see also my comment above about touch targets: #1317 (comment)
lib/widgets/message_list.dart
Outdated
return GestureDetector( | ||
behavior: HitTestBehavior.translucent, | ||
onLongPress: () { | ||
showChannelActionSheet(context, channelId: streamId); | ||
}, | ||
child: _buildStreamRow(context, stream: stream), | ||
); |
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: no newline before final closing )
.
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.
Also this needs test coverage.
lib/widgets/message_list.dart
Outdated
GestureDetector( | ||
behavior: HitTestBehavior.translucent, | ||
onLongPress: () { | ||
showChannelActionSheet(context, channelId: streamId); | ||
}, | ||
child: _buildStreamRow(context, stream: stream), | ||
), |
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.
Same comment about final closing )
and test coverage.
@@ -1083,6 +1093,7 @@ class StreamMessageRecipientHeader extends StatelessWidget { | |||
onTap: () => Navigator.push(context, | |||
MessageListPage.buildRoute(context: context, | |||
narrow: ChannelNarrow(message.streamId))), | |||
onLongPress: () => showChannelActionSheet(context, channelId: message.streamId), |
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 needs test coverage.
lib/widgets/action_sheet.dart
Outdated
optionButtons.add( | ||
MarkChannelAsReadButton( | ||
streamId: channelId, | ||
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.
optionButtons.add( | |
MarkChannelAsReadButton( | |
streamId: channelId, | |
pageContext: pageContext, | |
), | |
); | |
optionButtons.add( | |
MarkChannelAsReadButton(pageContext: pageContext, streamId: channelId)); |
(context first, and fewer lines of code)
fe82a57
to
85bd888
Compare
@chrisbobbe Pushed the revision. Please take a look. |
04d8daa
to
680f54f
Compare
It turns out that the |
Figma doesn't specify this icon explicitly. Based on discussions in PR zulip#1274 (zulip#1274 (comment)), we chose this icon for consistency with the "Mark as read" option on web. reference: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3512-7&t=iwirdCVutQls9R0A-4
680f54f
to
391bc6e
Compare
I've rebased past it and pushed the updated changes. Let me know if anything else is needed! |
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! Small comments below.
lib/widgets/action_sheet.dart
Outdated
final unreadCount = store.unreads.countInChannelNarrow(channelId); | ||
if (unreadCount > 0) { | ||
optionButtons.add( | ||
MarkChannelAsReadButton(pageContext: pageContext,streamId: channelId)); |
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: space after comma
@@ -1083,6 +1094,7 @@ class StreamMessageRecipientHeader extends StatelessWidget { | |||
onTap: () => Navigator.push(context, | |||
MessageListPage.buildRoute(context: context, | |||
narrow: ChannelNarrow(message.streamId))), | |||
onLongPress: () => showChannelActionSheet(context, channelId: message.streamId), |
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.
I've noticed an existing bug with the gesture handling here. I filed it as #1368; no need to fix it in this PR (but feel free to claim the issue if you'd like to work on it as a followup).
test/widgets/action_sheet_test.dart
Outdated
final effectiveChannel = channel ?? someChannel; | ||
final effectiveMessages = messages ?? [someMessage]; |
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.
Actually I think we can just do
final effectiveChannel = channel ?? someChannel; | |
final effectiveMessages = messages ?? [someMessage]; | |
channel ??= someChannel; | |
messages ??= [someMessage]; |
(here and in other added tests)
See Greg's comment here: #1029 (comment)
test/widgets/action_sheet_test.dart
Outdated
checkButtons(); | ||
}); | ||
|
||
testWidgets('show channel action sheet from recipient header stream row', (tester) async { |
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.
testWidgets('show channel action sheet from recipient header stream row', (tester) async { | |
testWidgets('show from recipient header', (tester) async { |
I think this is clear enough from context, and is fewer words.
test/widgets/action_sheet_test.dart
Outdated
// Prepare error response | ||
connection.prepare(httpException: http.ClientException('Oops')); | ||
|
||
// Tap and wait for dialog | ||
await tester.tap(findButtonForLabel('Mark channel as read')); | ||
await tester.pumpAndSettle(); // Wait for dialog animation |
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.
The code comments don't really add anything that isn't already clear; let's remove them.
391bc6e
to
622ac32
Compare
@chrisbobbe Pushed a revision. PTAL! |
Pull Request
Description
Adds a channel action sheet with a "Mark channel as read" option, accessible via long-press from both the inbox and subscription list pages. The action sheet is triggered by long-pressing on a channel header.
Related Issues
Screenshots
Additional Information
Currently the action sheet only appears in subscription list when the channel has unread messages, since the sheet doesn't have any default buttons.