channel_list: All channels screen

[Khader-1]

Fixes: #188

Link to all channels	Link when there is no other channels	All channels screen	When there's no channels
Subscribe success feedback	Already subscribed feedback	failed to subscribe feedback	failed to subscribe feedback due to exception
Unsubscribe success feedback	failed to unsubscribe feedback	failed to unsubscribe due to exception

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-28.at.17.56.09.mp4

[rajveermalviya]

Thanks for working on this @Khader-1!

Overall looks great, just some comments.

[Khader-1]

Thanks @rajveermalviya! I've pushed a new revision PTAL.

[rajveermalviya]

Thanks for the revision!
This looks good to me, moving on to the mentor review from @kenclary.

[kenclary]

lgtm

[alya]

Let's keep the empty view message consistent with the web app, unless there's a specific reason to do otherwise. So, it would be:

There are no channels you can view in this organization.

[alya]

We use sentence case throughout the Zulip app. So, "All channels", rather than "All Channels".

[alya]

Can you please include screenshots with the subscribed/unsubscribed messages? It's difficult to review text on a video.

[Khader-1]

Thanks @rajveermalviya, @kenclary and @alya for the reviews! Moving on for maintainer review now.
I've pushed a new revision and updated appended screenshots, PTAL @alya.

[gnprice]

-    channel_list: Setup subscribeToChannels api binding
+    api: Add route subscribeToChannels

(Thanks for making this update! I definitely appreciate when someone takes a comment from one PR and proactively applies it to other open PRs where it'd be relevant.)

[chrisbobbe]

@alya some of the snackbar messages have exclamation marks in them; do we want to keep those? Not sure what our pattern is for ending punctuation. Maybe they should even all have a period.

[chrisbobbe]

Thanks! Comments below.

Also, since we now have a dark theme (since #867 🎉), please update this to provide a dark-theme variant, and post screenshots of that. Since there isn't something we can really follow in the Figma or the web app, I expect each color will need one of these TODOs:

// TODO(design-dark) need proper dark-theme color (this is ad hoc)

(if you've chosen the dark-theme color yourself)

or

// TODO(design) check if this is the right variable

(if you've used a Figma design variable)

For examples, see c6abaf9.

[PIG208]

When first reading this, I couldn't understand the =0{all channels} variant within the context of this description, until I look at where this text appears in the app. I would expect something like =0{no other channels} by looking at this description alone.

I think we should mention that this is the label for the button that shows all channels.

[PIG208]

Because now we have a separate rule, the =0 variant can be dropped.

[PIG208]

Read the commits up to d16b58f, and left some comments.

[alya]

For the confirmation notices:

1. We have a pattern of always putting either a privacy icon (this is best if it's not too tricky), or at least a #, before the name of a channel.
2. Where did the strings for these notices come from? I would probably tweak some of them, but would like to keep them consistent if there's a source for them. At a minimum, we should consistently put punctuation at the end -- the unsuccessful ones are missing a ..

[alya]

For the exception ones, it seems like we're showing too much to the user. If it's not going to cause us difficulties investigating, I'd leave off the everything in the parentheses.

[chrisbobbe]

Thanks for the revision! Some small nits from me, then I think @PIG208 will want to check this new revision against his review. Also see Alya's open comments at #832 (comment) and right after.

[chrisbobbe]

Suggested change

	color: designVariables.background,
	// TODO(design) check if this is the right variable
	color: designVariables.background,

[chrisbobbe]

Suggested change

	? zulipLocalizations.browseMoreNChannels(notShownStreams)
	? zulipLocalizations.browseNMoreChannels(notShownStreams)

(sounds more natural, given that the string could be e.g. "Browse 6 more channels".)

[chrisbobbe]

Suggested change

	const SizedBox(width: 8),
	_ChannelItemSubscriptionToggle(stream: stream, channelItemContext: context),
	const SizedBox(width: 8),
	_ChannelItemSubscriptionToggle(stream: stream, channelItemContext: context),

[PIG208]

nit:

Suggested change

	check(find.byType(ChannelItem).evaluate().length).equals(expectedCount);
	check(find.byType(ChannelItem).evaluate()).length.equals(expectedCount);

[PIG208]

Thanks for the revision! Just took a quick look again. Reopened two previous comments and left another small one.

[Khader-1]

Thanks @alya, @chrisbobbe and @PIG208 for the reviews!

We have a pattern of always putting either a privacy icon (this is best if it's not too tricky), or at least a #, before the name of a channel.

I tried to use privacy icon but I could not find a direct way to inline the icon within the message text with proper wrapping. I'd love to hear any suggestion from Chris or Zixuan on how to do that.

Where did the strings for these notices come from?

I wrote them down. I tried to get the same as strings as mobile but I believe mobile shows a snackbar only if it fails to subscribe.

[alya]

I wrote them down. I tried to get the same as strings as mobile but I believe mobile shows a snackbar only if it fails to subscribe.

Ok, please start a discussion in #mobile regarding what these strings should be. You can post a list of situations where messages are shown and your suggested strings (with . added, since we definitely want that).

[chrisbobbe]

I tried to use privacy icon but I could not find a direct way to inline the icon within the message text with proper wrapping. I'd love to hear any suggestion from Chris or Zixuan on how to do that.

Try using a Text.rich instead of a Text, and put the privacy icon in a WidgetSpan. We have a few uses of WidgetSpan in message content: user mentions, image emojis, and global times.