Offer subscribe/unsubscribe in channel action sheet #1790

chrisbobbe · 2025-08-05T21:05:45Z

Stacked atop #1789.

We'll need API bindings for subscribe/unsubscribe for #188, so might as well knock out #1224 now that we have those.

Putting the context-menu buttons in groups, with spacing in between, is new here, and follows the Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6327-96691&m=dev

The Figma varies in whether to put 10px or 8px space between the sections, even within one context menu: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6098-6519&m=dev

and I've opted for 8px.

Screenshots

When unsubscribing from a private channel, a confirmation dialog:

with a followup to say "will not" instead of "might not", blocked on being able to check the group-based can-subscribe permission (#1786, #814).

rajveermalviya

Thanks @chrisbobbe! This LGTM, and tests well.

One (probably unrelated to this PR) thing I noticed is that currently when sending a message to an unsubscribed channel, the local echo progress bar is persistent. Even though on the Web app I notice that the message was acutally sent.

chrisbobbe · 2025-08-06T20:19:49Z

Yep, I've got a draft branch to address that :) foreshadowed in #1792.

alya · 2025-08-06T22:10:00Z

Is the context for the "might not" warning (vs. "will not" on web) that we aren't checking the can-subscribe permission here?

chrisbobbe · 2025-08-06T22:11:25Z

Exactly 🙂

When unsubscribing from a private channel, a confirmation dialog:

[screenshot]

with a followup to say "will not" instead of "might not", blocked on being able to check the group-based can-subscribe permission (#1786, #814).

gnprice

Thanks for building this! Generally this all looks good.

I'm not convinced that we want this rename commit:
648ab7f action_sheet [nfc]: Renames/doc changes to fit context-menu Figma component

I think so far "context menu" has been pretty much a synonym of "action sheet" for us. The "context menu" term appears in Figma, and in the names of design variables because we keep those aligned with Figma, but everywhere else we say "action sheet".

It'd be nice to have one term instead of two, or else to identify a useful distinction and then assign the two terms consistently to two related meanings. But to get to having just one term requires doing a quite comprehensive rename. With this rename commit, it feels like we still have two terms but the boundary between where they appear is less crisp than it is in the status quo.

gnprice · 2025-08-06T23:22:00Z

lib/api/route/channels.dart

+  List<int>? principals,
+}) {
+  return connection.post('subscribeToChannel', (_) {}, 'users/me/subscriptions', {
+    'subscriptions': subscriptions.map((subName) => {'name': subName}).toList(),


nit: the name "name" seems clear enough, and avoids making up a distinct name for the same thing:

Suggested change

'subscriptions': subscriptions.map((subName) => {'name': subName}).toList(),

'subscriptions': subscriptions.map((name) => {'name': name}).toList(),

gnprice · 2025-08-06T23:24:03Z

lib/widgets/action_sheet.dart

@@ -34,10 +34,14 @@ import 'text.dart';
 import 'theme.dart';
 import 'topic_list.dart';

-void _showActionSheet(


nit:

The Figma has a "context menu" component (see link in new dartdoc), which _showBottomSheet has been a good fit for. (See e.g.

should this say "_showActionSheet"?

gnprice · 2025-08-06T23:28:28Z

lib/widgets/action_sheet.dart

+/// A button in a context menu.
 ///
 /// When built from server data, the action sheet ignores changes in that data;


I think the term "action sheet" is going to remain in some places. The term "bottom sheet" will too, since it refers to the Material concept we're using.

So if we're going to introduce this third term, it'd be good to write down somewhere how the three different terms relate to each other. Perhaps on _showContextMenu? And/or here on ContextMenuItemButton, or have a reference here.

gnprice · 2025-08-06T23:30:02Z

lib/widgets/action_sheet.dart

-  required List<Widget> optionButtons,
+  required List<List<Widget>> buttonSections,


nit:

Some context menus in the Figma have multiple groups/sections of buttons, e.g.: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6327-96694&m=dev

please mention which menu this is, too — Figma is super slow to load, so I'd rather not have to follow the link to learn the answer when it's something that could be conveyed easily in text

gnprice · 2025-08-06T23:33:06Z

lib/widgets/action_sheet.dart

-    Navigator.push(pageContext,
-      TopicListPage.buildRoute(context: pageContext, streamId: channelId));
+  void onPressed() async {
+    final zulipLocalizations = ZulipLocalizations.of(pageContext);


nit: defer this lookup until we need the data (since that only happens on error)

And since it's across an async gap :)

Indeed. For localizations like this, that's basically harmless, which is why I didn't flag that reason — the potential impact is that if you changed your language settings while the request was ongoing, you'll see a message in your previously-chosen language. (Unlike the store, the old localizations object doesn't get disposed.)

But in principle that's also a good reason, and in the case of the store it'd be important.

chrisbobbe · 2025-08-07T00:15:43Z

Thanks! Revision pushed, and I removed that rename commit. I think my thought was: the Figma has a special kind of action sheet called a context menu, which is different from other action sheets, and it has these elements:

Optional header, with a few ways that can look
One or more sections of buttons, which are purple or (if destructive) red; this is our ZulipMenuItemButtonStyle.menu as opposed to .list
A cancel button at the bottom

And I was thinking about code organization around that pattern, just for the things that are actually meant to follow it:

Message, channel, topic, and DM action sheets
Uh, that might be it :)

gnprice · 2025-08-07T00:24:32Z

Cool.

Message, channel, topic, and DM action sheets

Uh, that might be it :)

That seems to align with "context menu" and "action sheet" being synonyms for us, then.

the Figma has a special kind of action sheet called a context menu, which is different from other action sheets,

A context menu, aka an action sheet, is still a specific kind of bottom sheet, though.

gnprice

Thanks! All looks good now except a couple of nits below.

gnprice · 2025-08-07T00:27:04Z

assets/l10n/app_en.arb

+  "@actionSheetOptionSubscribe": {
+    "description": "Label in the channel context menu for subscribing to the channel."


Let's stick to "action sheet" in these descriptions (here and another below)

gnprice · 2025-08-07T00:27:34Z

lib/widgets/action_sheet.dart

-                            child: MenuButtonsShape(buttons: optionButtons)))),
+                            child: Column(


nit:

action_sheet [nfc]: Support multiple button sections in context menu Some context menus in the Figma have multiple groups/sections of

s/context menu/action sheet/g

chrisbobbe · 2025-08-07T00:32:39Z

Ah indeed, basically an incomplete removal of that rename commit :) revision pushed.

Some action sheets in the Figma have multiple groups/sections of buttons, like in the channel action sheet, which has e.g. "Mark all messages as read" in one section, "Channel settings" in another, and "Unsubscribe" in its own one-item section: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6327-96694&m=dev

Figma link: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6091-50325&m=dev

… sheet Fixes zulip#1224.

gnprice · 2025-08-07T00:42:06Z

Thanks! Looks good; merging.

These were added yesterday in 7f7aa98 (zulip#1790). The descriptions changed in the last revision before the PR was merged; but the edit apparently didn't make it to the generated file, causing CI to fail.

chrisbobbe requested review from gnprice and rajveermalviya August 6, 2025 00:07

chrisbobbe assigned rajveermalviya Aug 6, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Aug 6, 2025

rajveermalviya approved these changes Aug 6, 2025

View reviewed changes

chrisbobbe assigned gnprice and unassigned rajveermalviya Aug 6, 2025

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Aug 6, 2025

gnprice reviewed Aug 6, 2025

View reviewed changes

chrisbobbe force-pushed the subscribe-unsubscribe-buttons branch from 253f4b9 to b907739 Compare August 7, 2025 00:09

gnprice reviewed Aug 7, 2025

View reviewed changes

chrisbobbe force-pushed the subscribe-unsubscribe-buttons branch from b907739 to 12b47e9 Compare August 7, 2025 00:32

chrisbobbe added 5 commits August 6, 2025 17:41

api: Add routes subscribeToChannel, unsubscribeFromChannel

e8928ae

action_sheet [nfc]: Refactor showChannelActionSheet button sections

81c2bb4

icons: Add circle_x, from Figma

e1d378c

Figma link: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6091-50325&m=dev

action_sheet: Add "Subscribe"/"Unsubscribe" buttons to channel action…

7f7aa98

… sheet Fixes zulip#1224.

gnprice force-pushed the subscribe-unsubscribe-buttons branch from 12b47e9 to 7f7aa98 Compare August 7, 2025 00:42

gnprice merged commit 7f7aa98 into zulip:main Aug 7, 2025
1 check failed

chrisbobbe deleted the subscribe-unsubscribe-buttons branch August 7, 2025 01:43

gnprice mentioned this pull request Aug 8, 2025

l10n: Fix mismatch in descriptions of new strings #1796

Merged

	'subscriptions': subscriptions.map((subName) => {'name': subName}).toList(),
	'subscriptions': subscriptions.map((name) => {'name': name}).toList(),

		required List<Widget> optionButtons,
		required List<List<Widget>> buttonSections,

		"@actionSheetOptionSubscribe": {
		"description": "Label in the channel context menu for subscribing to the channel."

		child: MenuButtonsShape(buttons: optionButtons)))),
		child: Column(

Offer subscribe/unsubscribe in channel action sheet #1790

Offer subscribe/unsubscribe in channel action sheet #1790

Uh oh!

Conversation

chrisbobbe commented Aug 5, 2025

Screenshots

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 6, 2025

Uh oh!

alya commented Aug 6, 2025

Uh oh!

chrisbobbe commented Aug 6, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 7, 2025

Uh oh!

gnprice commented Aug 7, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Aug 7, 2025

Uh oh!

gnprice commented Aug 7, 2025

Uh oh!

Uh oh!

Uh oh!