Subscriptions page #397

sirpengi · 2023-11-20T16:30:46Z

Adds a Subscriptions page showing the user's subscribed streams. Out of scope are collapsible topics and the ability to browse to all streams. There was no design spec for the situation where the user has no subscriptions so implemented a simple text notice here.

Thanks @chrisbobbe for the color helpers. I've found a lot that could be shared between this and the inbox code in #381 -- besides the collapsible bit, the design is exactly the same save the fontSize here is 18 instead of 17 in the inbox. I think we have a lot of space to consolidate design in the future.

Figma design: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?type=design&node-id=171-12359&mode=design&t=4d0vykoYQ0KGpFuu-0

Fixes #187

sirpengi · 2023-11-20T18:28:56Z

Having mulled it over after creating the pr I decided to rename iconDataFromStream to iconDataForStream

gnprice

Thanks @sirpengi! Generally this looks good; small comments below.

There is one puzzle in how this UI comes out. On my phone (a Pixel 8 running Android 14), the bold text comes out much bolder, like this:

Concretely, take a look at the "n"s in "announce". In this screenshot, the vertical strokes of the "n" are about as wide as the space enclosed in the "n", and perhaps a bit wider than the space between the two "n"s. But in both the Figma design and your screenshots, the strokes are a bit narrower than the space between the "n"s, and quite a bit narrower than the space within the "n".

Or look at the "a" in "announce" — in my screenshot, it's nearly closed up solid (and on the actual phone screen it feels even more so), while in both Figma and your screenshot, the white space enclosed in the bottom part of the "a" remains quite open.

The weight I'm seeing feels rather too bold, though the one in your screenshot looks perfectly fine. Doesn't have to block merging this, but it'll be a puzzle we'll want to follow up on.

It looks like your screenshots are from iOS. Is that right? Perhaps that's the variable that triggers the different behavior I'm seeing vs. what you're seeing.

gnprice · 2023-11-20T20:51:47Z

:q

@@ -0,0 +1,39 @@
+[1mdiff --git lib/widgets/icons.dart lib/widgets/icons.dart[m


:q | 39 +++++++++++++++++++++++++++++++++++++++

Seems like not what you intended :-)

gnprice · 2023-11-20T21:01:08Z

lib/widgets/subscription_list.dart

+  @override
+  void onNewStore() {
+    final store = PerAccountStoreWidget.of(context);
+    subscriptions = store.subscriptions;


For data like this that lives directly on the store (and for which updates are announced by the store itself, rather than by their own listeners like for store.unreads), let's stick to the usual pattern: the build method calls PerAccountStoreWidget.of for itself, and gets the data from the store.

gnprice · 2023-11-20T21:02:13Z

lib/widgets/subscription_list.dart

+      body: Builder(
+        builder: (BuildContext context) => Center(


Hmm interesting, what leads to the need for a Builder?

In fact it looks like this context doesn't actually end up getting used, so I think you can just cut the Builder.

gnprice · 2023-11-20T21:04:17Z

lib/widgets/subscription_list.dart

+                _SubscriptionList(unreadsModel: unreadsModel, subscriptions: unpinned),
+              ],
+
+              // TODO(#188): add button to "All Streams" page with ability to subscribe


Suggested change

// TODO(#188): add button to "All Streams" page with ability to subscribe

// TODO(#188): add button leading to "All Streams" page with ability to subscribe

Otherwise this keeps reading to me as about adding to such a page such a button.

gnprice · 2023-11-20T21:16:41Z

lib/widgets/subscription_list.dart

+                  fontSize: 14,
+                  letterSpacing: 0.56,
+                  height: (16 / 14),


The letter spacing appears in the Figma as "4%" in some places, and "0.56px" in others. The former sounds a lot more likely to me to be the way that it's expressed in Vlad's head as the author of the design — i.e. that's the source-code form, the form one would think of it in if one was adjusting it to a different value.

So let's express it that way here: 0.04 * 14.

This is similar to how we express the line height as 16 / 14 instead of like 1.1429, because "16px" is the way one would think of the design when thinking of editing it.

gnprice · 2023-11-20T21:18:00Z

lib/widgets/subscription_list.dart

+      child: InkWell(
+        onTap: () {
+          Navigator.push(context,
+            MessageListPage.buildRoute(context: context, narrow: StreamNarrow(subscription.streamId)));


nit:

Suggested change

MessageListPage.buildRoute(context: context, narrow: StreamNarrow(subscription.streamId)));

MessageListPage.buildRoute(context: context,

narrow: StreamNarrow(subscription.streamId)));

gnprice · 2023-11-20T21:18:37Z

lib/widgets/subscription_list.dart

+          Navigator.push(context,
+            MessageListPage.buildRoute(context: context, narrow: StreamNarrow(subscription.streamId)));
+        },
+        child: SizedBox(height: 40,


Probably a min-height is better, to accommodate large font sizes.

As we talked in our pairing yesterday, I've switched this to not have a defined height, but added padding to the elements in this so that it results in 40px (and the _SubscriptionListHeader similarly for 30px) so that they can expand if the font size is larger.

gnprice

Ah and I forgot to look at the tests before posting the review above. (Because the screenshot caused me to start writing in the "finish your review" box.) Read those now; a couple of small comments below.

gnprice · 2023-11-20T21:45:46Z

test/widgets/subscription_list_test.dart

+    // create simple versions from subscriptions
+    final List<ZulipStream> streams = subscriptions.map(
+      (e) => eg.stream(streamId: e.streamId, name: e.name)).toList();


Suggested change

// create simple versions from subscriptions

final List<ZulipStream> streams = subscriptions.map(

(e) => eg.stream(streamId: e.streamId, name: e.name)).toList();

final List<ZulipStream> streams = subscriptions.toList();

right? 😁

(And then inline that to the line below that uses it.)

gnprice · 2023-11-20T21:49:06Z

test/widgets/subscription_list_test.dart

+    check(isUnpinnedHeaderInTree()).isTrue();
+
+    final streamListItems = tester.widgetList<SubscriptionItem>(find.byType(SubscriptionItem)).toList();
+    check(streamListItems.length).equals(4);


Suggested change

check(streamListItems.length).equals(4);

check(streamListItems).length.equals(4);

Using the check helpers makes the errors more informative if something breaks the test.

gnprice · 2023-11-20T21:55:20Z

test/widgets/subscription_list_test.dart

+    check(streamListItems[0].subscription.streamId).equals(3);
+    check(streamListItems[1].subscription.streamId).equals(1);
+    check(streamListItems[2].subscription.streamId).equals(4);
+    check(streamListItems[3].subscription.streamId).equals(2);


Similarly here: imagine writing a change that turns out to break this test. This is basically the moment where the test fulfills its purpose — the point of a test is that if we write a change that breaks it, the test tells us there's something wrong with our change, or at least something that contradicts a previous expectation and we need to revise the expectation. So the value of the test is tied to how well it tells us what it is that's wrong or unexpected about our change.

Here, that mission will be much better accomplished if the test failure message tells us about the full list of these streams, rather than just one — the full list is how we can see how the sorting ended up and how that differs from what was expected.

So let's get the observed result into the form of a list that we can compare with deepEquals. Could be like check(streamListItems.map(…)).deepEquals([3, 1, 4, 2]).

sirpengi · 2023-11-21T14:47:04Z

@gnprice revision ready!

gnprice

Thanks! Quite close now; a handful of comments below.

gnprice · 2023-11-21T16:17:22Z

lib/widgets/subscription_list.dart

+            if (pinned.isEmpty && unpinned.isEmpty)
+              const _NoSubscriptionsItem(),
+            if (pinned.isNotEmpty) ...[
+              _SubscriptionListHeader(context: context, label: "Pinned"),


It's a bit of an odd pattern to put a BuildContext as a property on a widget — after all, the widget will have its own context when it gets built. So giving it a context as a property can only make sense if there's some particular way that that other context is expected to be different from the one it's built at; which will then also mean the name should be something more specific than "context".

I think the one example of that pattern that we currently have in our codebase is MessageActionSheetMenuItemButton.messageListContext.

Here, as mentioned at #397 (comment) , it looks like this context doesn't end up getting used. So we can just leave it out.

I think this was an artifact from refactoring this from a helper method to a widget of its own. I'll be more mindful of this!

gnprice · 2023-11-21T16:38:01Z

lib/widgets/subscription_list.dart

+              iconDataForStream(subscription)),
+          ),


nit:

Suggested change

iconDataForStream(subscription)),

),

iconDataForStream(subscription))),

gnprice · 2023-11-21T16:41:54Z

test/widgets/subscription_list_test.dart

+    check(streamListItems).length.equals(4);
+    check(streamListItems.map((e) => e.subscription.streamId)).deepEquals([3, 1, 4, 2]);


Suggested change

check(streamListItems).length.equals(4);

check(streamListItems.map((e) => e.subscription.streamId)).deepEquals([3, 1, 4, 2]);

check(streamListItems.map((e) => e.subscription.streamId))

.deepEquals([3, 1, 4, 2]);

The length check is redundant once you're doing deepEquals on the list.

sirpengi · 2023-11-21T16:53:17Z

@gnprice ready again!

gnprice · 2023-11-21T16:57:10Z

(It looks like there isn't yet a new version pushed.)

chrisbobbe

Hi! 👋 I had a few small comments while reading through this just now; see below. Hopefully this doesn't interrupt too much the conversations here between you and Greg.

Another thought I had, from comparing with the page on Figma, is that I'm not yet sure if only some of the stream rows should use bold text (as in this revision), or if all of them should. The Figma shows all of the stream rows in bold:

But it doesn't show any streams without unreads. So I think it's inconclusive about whether bold text is used to distinguish streams with unreads from streams without unreads, or if it's used to distinguish stream rows from topic rows. (I understand that topic rows is being saved for future work; there's more complicated data stuff we need for that.)

I guess one observation that might hint away from interpreting the use of bold to distinguish unreads: in the Figma, there are some topic rows with unreads, and some without unreads, and none of the ones with unreads are bold.

If you agree, I think you could just leave a TODO pointing to this comment and we can revisit it later, with feedback from Vlad.

chrisbobbe · 2023-11-21T16:49:25Z

lib/widgets/icons.dart

+  // TODO: these icons aren't quite right yet; see:
+  //   https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20.23F117.20.22Inbox.22.20screen/near/1680637


The original comment had this:

see this message and the following:

Another way to say what I meant is "see this message and the one after it", and I think that way would have been clearer 😅. Anyway, I'd like to keep the explicitness that there are two relevant Zulip messages. The linked one is pretty long, and the much shorter one just after it might be easy to overlook in a hurry.

I changed it to "see this message and the one after it" to be more specific. I originally removed "and the following" as I interpreted it as "and all the following messages" which seemed superfluous.

chrisbobbe · 2023-11-21T16:51:10Z

lib/widgets/subscription_list.dart

@@ -0,0 +1,241 @@
+


nit: blank line not needed

chrisbobbe · 2023-11-21T16:53:16Z

lib/widgets/subscription_list.dart

+  void _modelChanged() {
+    setState(() {
+      // The actual state lives in [subscriptions] and [unreadsModel].
+      // This method was called because one of those just changed.
+    });
+  }


nit: This won't be called when [subscriptions] changes (and it doesn't need to be), so this should just say [unreadsModel].

This will be called when subscriptions changes (though technically on any store update). The page updates if you pin/unpin or subscribe/unsubscribe to a stream.

The page does update when you pin/unpin or subscribe/unsubscribe, but _modelChanged isn't involved in making that happen. It happens because PerAccountStoreWidget.of(context) is called in the build method. From the dartdoc on PerAccountStoreWidget.of(context):

/// The given build context will be registered as a dependency of the /// returned store. This means that when the data in the store changes, /// the element at that build context will be rebuilt.

Makes sense! Updated as suggested

chrisbobbe · 2023-11-21T16:55:40Z

lib/widgets/subscription_list.dart

+          color: const HSLColor.fromAHSL(1.0, 240, 0.1, 0.5).toColor(),
+          fontFamily: 'Source Sans 3',
+          fontSize: 18,
+          height: (20 / 18),
+        ).merge(weightVariableTextStyle(context)))));


nit: indentation

sirpengi · 2023-11-21T19:56:33Z

Hi! 👋 I had a few small comments while reading through this just now; see below. Hopefully this doesn't interrupt too much the conversations here between you and Greg.

Another thought I had, from comparing with the page on Figma, is that I'm not yet sure if only some of the stream rows should use bold text (as in this revision), or if all of them should. The Figma shows all of the stream rows in bold:

But it doesn't show any streams without unreads. So I think it's inconclusive about whether bold text is used to distinguish streams with unreads from streams without unreads, or if it's used to distinguish stream rows from topic rows. (I understand that topic rows is being saved for future work; there's more complicated data stuff we need for that.)

I guess one observation that might hint away from interpreting the use of bold to distinguish unreads: in the Figma, there are some topic rows with unreads, and some without unreads, and none of the ones with unreads are bold.

If you agree, I think you could just leave a TODO pointing to this comment and we can revisit it later, with feedback from Vlad.

I agree that the design specs are unclear. I had settled on this due to the existence of this outdated design in the same figma that shows streams with unreads in bold:

(from https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=112%3A9610&mode=dev )

I've added a TODO and linked to your comment as suggested.

Fixes: zulip#187

sirpengi · 2023-11-21T23:17:17Z

Thanks @chrisbobbe for the review, your comments should be handled now!

gnprice · 2023-11-21T23:56:57Z

Thanks @sirpengi for the revisions, and thanks @chrisbobbe for the added review!

All now looks good; merging.

sirpengi requested a review from gnprice November 20, 2023 17:36

sirpengi force-pushed the pr-subscriptions-page branch from f0148ac to 5fdf082 Compare November 20, 2023 18:29

gnprice reviewed Nov 20, 2023

View reviewed changes

sirpengi force-pushed the pr-subscriptions-page branch 2 times, most recently from b072577 to ac13fd3 Compare November 21, 2023 14:37

gnprice reviewed Nov 21, 2023

View reviewed changes

chrisbobbe reviewed Nov 21, 2023

View reviewed changes

sirpengi force-pushed the pr-subscriptions-page branch from ac13fd3 to 21d4849 Compare November 21, 2023 17:55

ui [nfc]: Pull out iconDataForStream helper

ef9d184

sirpengi force-pushed the pr-subscriptions-page branch from 21d4849 to 6d80be5 Compare November 21, 2023 20:09

subscription_list: Add new SubscriptionListPage

e4e2e88

Fixes: zulip#187

sirpengi force-pushed the pr-subscriptions-page branch from 6d80be5 to e4e2e88 Compare November 21, 2023 23:14

gnprice merged commit e4e2e88 into zulip:main Nov 21, 2023
1 check passed

sirpengi deleted the pr-subscriptions-page branch January 23, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscriptions page #397

Subscriptions page #397

sirpengi commented Nov 20, 2023 •

edited

Loading

sirpengi commented Nov 20, 2023

gnprice left a comment

gnprice Nov 20, 2023

gnprice Nov 20, 2023

gnprice Nov 20, 2023

gnprice Nov 20, 2023

gnprice Nov 20, 2023

gnprice Nov 20, 2023

gnprice Nov 20, 2023

sirpengi Nov 21, 2023

gnprice left a comment

gnprice Nov 20, 2023

gnprice Nov 20, 2023

gnprice Nov 20, 2023

sirpengi commented Nov 21, 2023

gnprice left a comment

gnprice Nov 21, 2023

sirpengi Nov 21, 2023

gnprice Nov 21, 2023

gnprice Nov 21, 2023

sirpengi commented Nov 21, 2023

gnprice commented Nov 21, 2023

chrisbobbe left a comment •

edited

Loading

chrisbobbe Nov 21, 2023

sirpengi Nov 21, 2023

chrisbobbe Nov 21, 2023

chrisbobbe Nov 21, 2023

sirpengi Nov 21, 2023

chrisbobbe Nov 21, 2023

sirpengi Nov 21, 2023

chrisbobbe Nov 21, 2023

sirpengi commented Nov 21, 2023

sirpengi commented Nov 21, 2023

gnprice commented Nov 21, 2023

		@@ -0,0 +1,39 @@
		[1mdiff --git lib/widgets/icons.dart lib/widgets/icons.dart[m

	// TODO(#188): add button to "All Streams" page with ability to subscribe
	// TODO(#188): add button leading to "All Streams" page with ability to subscribe

	MessageListPage.buildRoute(context: context, narrow: StreamNarrow(subscription.streamId)));
	MessageListPage.buildRoute(context: context,
	narrow: StreamNarrow(subscription.streamId)));

	check(streamListItems.length).equals(4);
	check(streamListItems).length.equals(4);

	iconDataForStream(subscription)),
	),
	iconDataForStream(subscription))),

		check(streamListItems).length.equals(4);
		check(streamListItems.map((e) => e.subscription.streamId)).deepEquals([3, 1, 4, 2]);

		// TODO: these icons aren't quite right yet; see:
		// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/design.3A.20.23F117.20.22Inbox.22.20screen/near/1680637

Subscriptions page #397

Subscriptions page #397

Conversation

sirpengi commented Nov 20, 2023 • edited Loading

sirpengi commented Nov 20, 2023

gnprice 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

gnprice 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

sirpengi commented Nov 21, 2023

gnprice 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

sirpengi commented Nov 21, 2023

gnprice commented Nov 21, 2023

chrisbobbe left a comment • edited Loading

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

sirpengi commented Nov 21, 2023

sirpengi commented Nov 21, 2023

gnprice commented Nov 21, 2023

sirpengi commented Nov 20, 2023 •

edited

Loading

chrisbobbe left a comment •

edited

Loading