Skip to content

all_channels: Replace three dot menu with long press gesture and add on tap to feed #1915

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+59 −29
Open

all_channels: Replace three dot menu with long press gesture and add on tap to feed #1915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d79bcb7
all_channels: Remove three dot menu and add gesture controls
chungwwei Oct 11, 2025
7e3f92e
all_channels: Restrict channel name to oneline
chungwwei Oct 26, 2025
baf929f
all_channels: Make row item at least 44px tall with ConstrainedBox
chungwwei Oct 26, 2025
5a60145
all_channels: Make toggle transparent and non-interactive when
chungwwei Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 30 additions & 22 deletions lib/widgets/all_channels.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import '../api/route/channels.dart';
import '../generated/l10n/zulip_localizations.dart';
import '../log.dart';
import '../model/channel.dart';
import '../model/narrow.dart';
import 'action_sheet.dart';
import 'actions.dart';
import 'app_bar.dart';
import 'button.dart';
import 'icons.dart';
import 'message_list.dart';
import 'page.dart';
import 'remote_settings.dart';
import 'store.dart';
Expand Down Expand Up @@ -96,28 +98,34 @@ class AllChannelsListEntry extends StatelessWidget {
final Subscription? subscription = channel is Subscription ? channel : null;
final hasContentAccess = store.selfHasContentAccess(channel);

return Padding(
padding: EdgeInsetsDirectional.only(start: 8, end: 4),
child: Row(spacing: 6, children: [
Icon(
size: 20,
color: colorSwatchFor(context, subscription).iconOnPlainBackground,
iconDataForStream(channel)),
Expanded(
child: Text(
style: TextStyle(
color: designVariables.textMessage,
fontSize: 17,
height: 20 / 17,
).merge(weightVariableTextStyle(context, wght: 600)),
channel.name)),
if (hasContentAccess) _SubscribeToggle(channel: channel),
ZulipIconButton(
icon: ZulipIcons.more_horizontal,
onPressed: () {
showChannelActionSheet(context, channelId: channel.streamId);
}),
]));
return InkWell(
onTap: !hasContentAccess ? null : () => Navigator.push(context,
MessageListPage.buildRoute(context: context,
narrow: ChannelNarrow(channel.streamId))),
onLongPress: () => showChannelActionSheet(context, channelId: channel.streamId),
child: ConstrainedBox(constraints: const BoxConstraints(minHeight: 44),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit-sequence nit:

all_channels: Make row item at least 44px tall with ConstrainedBox

Make sure every row is at least 44px in height by specifying
BoxConstraints(minHeight: 44) to meet touch target requirement.

Did the removal of the three-dots button make these rows shorter than they had been?

If so, then this commit adding the 44px min-height should go before the commit that removes the three-dots button. That way we don't have an intermediate state where the UI is less good in some ways than it was before.

child: Padding(
padding: EdgeInsetsDirectional.only(start: 8, end: 12),
child: Row(spacing: 6, children: [
Icon(
size: 20,
color: colorSwatchFor(context, subscription).iconOnPlainBackground,
iconDataForStream(channel)),
Expanded(
child: Text(
maxLines: 1,
overflow: TextOverflow.ellipsis,
Comment on lines +116 to +117
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

all_channels: Restrict channel name to oneline

"one line" is two words; "oneline" isn't a word

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separate nit:

Fixes part of #1914

If this is the commit that fixes the last remaining part of #1914, then it's the commit that causes #1914 to become fixed. So in that case the commit message should say that it fixes #1914.

style: TextStyle(
color: designVariables.textMessage,
fontSize: 17,
height: 20 / 17,
).merge(weightVariableTextStyle(context, wght: 600)),
channel.name)),
IgnorePointer(ignoring: !hasContentAccess,
child: AnimatedOpacity(opacity: !hasContentAccess ? 0 : 1,
duration: Duration(milliseconds: 200),
child: _SubscribeToggle(channel: channel)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: always use trailing comma, unless ) is on the same line

Suggested change
child: _SubscribeToggle(channel: channel)))
child: _SubscribeToggle(channel: channel))),

(This will also slightly reduce the diff in the third / next-to-last commit.)

Comment on lines +124 to +127
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced we need this change, i.e. the last commit:
5a60145 all_channels: Make toggle transparent and non-interactive when noContentAccess

It's not a super common interaction in the first place (though it is an important one) to subscribe and unsubscribe to channels; and I think it's going to be a lot less common than that to unsubscribe when that means you lose access to the channel. So if that sometimes means that the row gets a bit shorter and the rows below jump up a bit, I think that's a pretty minor issue.

(It's also not clear to me whether that ever actually does happen, or if it's a theoretical issue.)

Conversely, having this here makes the code somewhat more complicated to understand. It also adds a nontrivial widget (the toggle) to the tree in cases where the user won't actually see it; and does so for each element of a potentially long list, so it has a potential performance cost.

]))));
}
}

Expand Down
36 changes: 29 additions & 7 deletions test/widgets/all_channels_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import 'package:zulip/widgets/app_bar.dart';
import 'package:zulip/widgets/button.dart';
import 'package:zulip/widgets/home.dart';
import 'package:zulip/widgets/icons.dart';
import 'package:zulip/widgets/message_list.dart';
import 'package:zulip/widgets/page.dart';
import 'package:zulip/widgets/remote_settings.dart';
import 'package:zulip/widgets/theme.dart';
Expand Down Expand Up @@ -194,29 +195,50 @@ void main() {

check(findInRow(find.text(channel.name))).findsOne();

final maybeToggle = tester.widgetList<Toggle>(
findInRow(find.byType(Toggle))).singleOrNull;
final toggle = tester.widget<Toggle>(findInRow(find.byType(Toggle)));
final opacityWidget = tester.widget<AnimatedOpacity>(findInRow(find.byType(AnimatedOpacity)));
final ignoreWidget = tester.widget<IgnorePointer>(findInRow(find.byType(IgnorePointer)));
if (store.selfHasContentAccess(channel)) {
final isSubscribed = channel is Subscription;
check(maybeToggle).isNotNull().value.equals(isSubscribed);
check(toggle).value.equals(isSubscribed);
check(opacityWidget.opacity).equals(1);
check(ignoreWidget.ignoring).isFalse();
} else {
check(maybeToggle).isNull();
check(opacityWidget.opacity).equals(0);
check(ignoreWidget.ignoring).isTrue();
}

check(findInRow(find.byIcon(ZulipIcons.more_horizontal))).findsOne();
final touchTargetSize = tester.getSize(findElement);
check(touchTargetSize.height).equals(44);
}
});

testWidgets('tapping three-dots button opens channel action sheet', (tester) async {
testWidgets('open channel action sheet on long press', (tester) async {
await setupAllChannelsPage(tester, channels: [eg.stream()]);

await tester.tap(find.byIcon(ZulipIcons.more_horizontal));
await tester.longPress(find.byType(AllChannelsListEntry));
await tester.pump();
await transitionDurationObserver.pumpPastTransition(tester);

check(find.byType(BottomSheet)).findsOne();
});

testWidgets('navigate to channel feed on tap', (tester) async {
final channel = eg.stream(name: 'some-channel');
await setupAllChannelsPage(tester, channels: [channel]);

connection.prepare(json: eg.newestGetMessagesResult(
foundOldest: true, messages: [eg.streamMessage(stream: channel)]).toJson());
await tester.tap(find.byType(AllChannelsListEntry));
await tester.pump();
await transitionDurationObserver.pumpPastTransition(tester);

check(find.descendant(
of: find.byType(MessageListPage),
matching: find.text('some-channel')),
).findsOne();
});

testWidgets('use toggle switch to subscribe/unsubscribe', (tester) async {
final channel = eg.stream();
await setupAllChannelsPage(tester, channels: [channel]);
Expand Down