channel_list: All channels screen #832

Khader-1 · 2024-07-23T13:23:47Z

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	Unsubscribe success feedback	failed to unsubscribe feedback

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.

test/widgets/subscription_list_test.dart

lib/widgets/channel_list.dart

lib/widgets/subscription_list.dart

test/widgets/subscription_list_test.dart

lib/widgets/channel_list.dart

Khader-1 · 2024-08-07T08:25:58Z

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.

assets/l10n/app_en.arb

kenclary

lgtm

alya · 2024-08-07T21:24:02Z

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 · 2024-08-07T21:25:58Z

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

alya · 2024-08-07T21:27:06Z

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

Khader-1 · 2024-08-08T09:48:40Z

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 · 2024-08-12T20:55:52Z

-    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 · 2024-08-12T22:56:28Z

@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.

assets/l10n/app_en.arb

lib/widgets/channel_list.dart

test/widgets/channel_list_test.dart

lib/widgets/channel_list.dart

assets/l10n/app_en.arb

lib/widgets/channel_list.dart

lib/widgets/subscription_list.dart

test/widgets/channel_list_test.dart

test/widgets/subscription_list_test.dart

test/widgets/channel_list_test.dart

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.

lib/widgets/channel_list.dart

lib/widgets/subscription_list.dart

lib/widgets/channel_list.dart

test/widgets/channel_list_test.dart

PIG208

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

Khader-1 · 2024-08-29T09:48:10Z

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 · 2024-08-29T16:39:26Z

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 · 2024-08-29T21:43:29Z

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.

This creates the page that contains all channels list, still we need to implement subscribe/unsubscribe logic which will be introduced in the upcoming commits. Fixes parts of zulip#188

Fixes: zulip#188

Khader-1 · 2024-11-19T21:07:01Z

Thanks @alya, @chrisbobbe and @PIG208! I'm finally back with another revision, PTAL.

PIG208

Glad to have you back @Khader-1! Thanks for the update! I have read through the commits and left some comments.

I think the last two commits can be squashed together:

f604530 channel_list: Implement unsubscribe from channel
793e9bf channel_list: Implement subscribe to channel

And similarly, these two can be squashed together:

cc676ac api: Add route unsubscribeFromChannels
96836fc api: Add route subscribeToChannels

PIG208 · 2024-09-17T19:18:16Z

assets/l10n/app_en.arb

@@ -367,6 +367,17 @@
      "num": {"type": "int", "example": "4"}


It would be great to have a link to the discussion on the design here, so that we know this UI is based on the legacy mobile app.

PIG208 · 2024-09-17T19:22:36Z

lib/model/channel.dart

@@ -262,6 +262,12 @@ class ChannelStoreImpl with ChannelStore {

      case SubscriptionRemoveEvent():
        for (final streamId in event.streamIds) {
+          final subscription = streams[streamId];
+          if (subscription == null || subscription is! Subscription) { // TODO(log)
+            continue;


Looks like this is only possible when the server behaves unexpectedly. So it might fit better in a non-nfc commit explaining how this can happen.

PIG208 · 2024-09-17T19:26:33Z

test/model/channel_test.dart

+        subscriptions: [eg.subscription(stream)],
+      ));
+
+      check(store.streams[stream.streamId] is Subscription).isTrue();


nit: use isA<Subscription> instead and we can skip the isTrue check

assets/l10n/app_en.arb

PIG208 · 2024-11-21T17:27:16Z

lib/widgets/subscription_list.dart

+              crossAxisAlignment: CrossAxisAlignment.center,
+              mainAxisAlignment: MainAxisAlignment.spaceBetween,
+              children: [
+                Text(


Let's make sure that the text wraps when there is not enough space. Wrapping this in Expanded should make it resize correctly.

PIG208 · 2024-11-21T18:01:01Z

test/widgets/channel_list_test.dart

+    Future<ZulipStream> prepareSingleStream(WidgetTester tester) async {
+        final stream = eg.stream();
+        await setupChannelListPage(tester, streams: [stream], subscriptions: []);
+        return stream;


nit: indentation

PIG208 · 2024-11-21T18:01:22Z

test/widgets/channel_list_test.dart

+      check(find.descendant(
+        of: richTextFinder,
+        matching: find.byIcon(ZulipIcons.hash_sign),
+      ).evaluate()).single;


nit: use findsOne as mentioned earlier

PIG208 · 2024-11-21T18:02:35Z

test/widgets/channel_list_test.dart

+      await setupChannelListPage(tester, streams: [stream], subscriptions: []);
+      connection.prepare(json: SubscribeToChannelsResult(
+        subscribed: {eg.selfUser.email: [stream.name]},
+        alreadySubscribed: {}).toJson());


We could add a delay to make this more realistic.

PIG208 · 2024-11-21T18:10:07Z

lib/widgets/channel_list.dart

+            height: (20 / 18),
+          ))));


Suggested change

height: (20 / 18),

))));

height: (20 / 18)))));

PIG208 · 2024-11-21T18:11:34Z

lib/widgets/subscription_list.dart

@@ -1,4 +1,5 @@
 import 'package:flutter/material.dart';
+import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';


We will need to regenerate the translation strings using tools/check l10n --fix, and update this import to use import '../generated/l10n/zulip_localizations.dart'; instead.

gnprice · 2024-12-18T22:33:54Z

Thanks again @Khader-1 for all your work on this PR!

Do you think you'll have time to make a revision based on Zixuan's feedback above?

gnprice · 2025-02-06T06:54:52Z

Marking this PR as draft for now to get it out of our review queue. @Khader-1 if in the future you're able to pick this up again and revise for the feedback above, feel free to mark again for review. Hope you're well.

Khader-1 force-pushed the all-streams-screen branch 4 times, most recently from e4fecd9 to a0c2bb2 Compare July 29, 2024 06:06

Khader-1 changed the title ~~[WIP] All streams screen~~ channel_list: All channels screen Jul 29, 2024

Khader-1 added the buddy review GSoC buddy review needed. label Jul 29, 2024

Khader-1 requested a review from rajveermalviya July 29, 2024 06:59

Khader-1 marked this pull request as ready for review July 29, 2024 06:59

Khader-1 force-pushed the all-streams-screen branch from a0c2bb2 to 9a12a3e Compare July 29, 2024 07:00

rajveermalviya requested changes Jul 30, 2024

View reviewed changes

Khader-1 force-pushed the all-streams-screen branch from 9a12a3e to 75d75c1 Compare August 7, 2024 08:24

rajveermalviya approved these changes Aug 7, 2024

View reviewed changes

assets/l10n/app_en.arb Outdated Show resolved Hide resolved

rajveermalviya added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Aug 7, 2024

rajveermalviya requested a review from kenclary August 7, 2024 08:58

kenclary approved these changes Aug 7, 2024

View reviewed changes

Khader-1 force-pushed the all-streams-screen branch from 75d75c1 to c567e9e Compare August 8, 2024 09:28

Khader-1 added maintainer review PR ready for review by Zulip maintainers and removed mentor review GSoC mentor review needed. labels Aug 8, 2024

Khader-1 force-pushed the all-streams-screen branch from c567e9e to d1a1c59 Compare August 12, 2024 11:24

chrisbobbe requested changes Aug 14, 2024

View reviewed changes

PIG208 reviewed Aug 14, 2024

View reviewed changes

test/widgets/channel_list_test.dart Outdated Show resolved Hide resolved

Khader-1 force-pushed the all-streams-screen branch 2 times, most recently from f816da7 to 41de1fc Compare August 22, 2024 11:14

chrisbobbe requested a review from PIG208 August 23, 2024 22:45

chrisbobbe reviewed Aug 23, 2024

View reviewed changes

lib/widgets/channel_list.dart Show resolved Hide resolved

lib/widgets/subscription_list.dart Outdated Show resolved Hide resolved

lib/widgets/channel_list.dart Outdated Show resolved Hide resolved

PIG208 reviewed Aug 26, 2024

View reviewed changes

test/widgets/channel_list_test.dart Outdated Show resolved Hide resolved

PIG208 reviewed Aug 26, 2024

View reviewed changes

Khader-1 force-pushed the all-streams-screen branch 2 times, most recently from 846174a to 368ad56 Compare August 29, 2024 09:34

gnprice assigned PIG208 Sep 12, 2024

Khader-1 added 6 commits November 19, 2024 21:49

channel [nfc]: Add assertions when handling SubscriptionRemoveEvent

be92ef5

channel: Ensure SubscriptionRemoveEvent updates all collections

33924e6

subscription_list test [nfc]: Remove unnecessary toList() call

18bb12a

channel_list: Create "All channels" page

f51e828

This creates the page that contains all channels list, still we need to implement subscribe/unsubscribe logic which will be introduced in the upcoming commits. Fixes parts of zulip#188

api: Add route subscribeToChannels

96836fc

api: Add route unsubscribeFromChannels

cc676ac

Khader-1 force-pushed the all-streams-screen branch 2 times, most recently from efd791d to b91bec1 Compare November 19, 2024 20:41

Khader-1 added 2 commits November 19, 2024 23:03

channel_list: Implement subscribe to channel

793e9bf

Fixes: zulip#188

channel_list: Implement unsubscribe from channel

f604530

Khader-1 force-pushed the all-streams-screen branch from b91bec1 to f604530 Compare November 19, 2024 21:03

Khader-1 requested a review from PIG208 November 19, 2024 21:05

PIG208 reviewed Nov 21, 2024

View reviewed changes

gnprice marked this pull request as draft February 6, 2025 06:54

gnprice added this to the M6: Post-launch milestone Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

channel_list: All channels screen #832

channel_list: All channels screen #832

Khader-1 commented Jul 23, 2024 •

edited

Loading

rajveermalviya left a comment

Khader-1 commented Aug 7, 2024

rajveermalviya left a comment

kenclary left a comment

alya commented Aug 7, 2024

alya commented Aug 7, 2024

alya commented Aug 7, 2024

Khader-1 commented Aug 8, 2024

gnprice commented Aug 12, 2024

chrisbobbe commented Aug 12, 2024

chrisbobbe left a comment •

edited

Loading

chrisbobbe left a comment •

edited

Loading

PIG208 left a comment

Khader-1 commented Aug 29, 2024

alya commented Aug 29, 2024

chrisbobbe commented Aug 29, 2024

Khader-1 commented Nov 19, 2024

PIG208 left a comment

PIG208 Sep 17, 2024

PIG208 Sep 17, 2024

PIG208 Sep 17, 2024

PIG208 Nov 21, 2024

PIG208 Nov 21, 2024

PIG208 Nov 21, 2024

PIG208 Nov 21, 2024

PIG208 Nov 21, 2024

PIG208 Nov 21, 2024

gnprice commented Dec 18, 2024

gnprice commented Feb 6, 2025

		@@ -1,4 +1,5 @@
		import 'package:flutter/material.dart';
		import 'package:flutter_gen/gen_l10n/zulip_localizations.dart';

channel_list: All channels screen #832

Are you sure you want to change the base?

channel_list: All channels screen #832

Conversation

Khader-1 commented Jul 23, 2024 • edited Loading

rajveermalviya left a comment

Choose a reason for hiding this comment

Khader-1 commented Aug 7, 2024

rajveermalviya left a comment

Choose a reason for hiding this comment

kenclary left a comment

Choose a reason for hiding this comment

alya commented Aug 7, 2024

alya commented Aug 7, 2024

alya commented Aug 7, 2024

Khader-1 commented Aug 8, 2024

gnprice commented Aug 12, 2024

chrisbobbe commented Aug 12, 2024

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

PIG208 left a comment

Choose a reason for hiding this comment

Khader-1 commented Aug 29, 2024

alya commented Aug 29, 2024

chrisbobbe commented Aug 29, 2024

Khader-1 commented Nov 19, 2024

PIG208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Dec 18, 2024

gnprice commented Feb 6, 2025

Khader-1 commented Jul 23, 2024 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe left a comment •

edited

Loading