all_channels: Implement "All channels" page #1849
Conversation
All callers pass an `onPressed`, and the behavior when it's not passed isn't coherent -- some aspects of the button take on a disabled appearance following Material defaults (e.g. the background is faded), but some don't, notably the icon color if an icon is present.
…ground The point of setting this at all is to suppress the outline, instead of painting it with a Material default. Now we're suppressing it in a way that will work if activeColor or inactiveColor is translucent -- the outline is painted over the track -- and we'd like to make those translucent for a disabled state, which we'll implement soon.
Vlad suggested using 0.5 opacity if no special disabled state is provided: https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/toggle.3A.20disabled.20state/near/2250883 but Greg and I prefer 0.4 in this case to make it more distinct. We may or may not end up using this for unsubscribed channels without content access (i.e. ones that you can't subscribe yourself to). We may instead just not show a toggle for those channels.
Fixes zulip#188. See Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=7723-6411&m=dev Notable features not included here: - Colorizing unsubscribed channels, zulip#1848 - "New channel", zulip#1572
|
There was a problem hiding this comment.
Thanks for working on this @chrisbobbe! Just one comment otherwise LGTM, also I see that one test failing CI currently.
| final items = channels.values.toList(); | ||
| _sort(items); |
There was a problem hiding this comment.
Should the result of this list be cached or initialization moved to onNewStore?
There was a problem hiding this comment.
Thanks for building this, and thanks @rajveermalviya for the previous review!
Generally this looks great. Various small comments below.
| "allChannelsEmptyPlaceholder": "There are no channels in this Zulip organization.", | ||
| "@allChannelsEmptyPlaceholder": { | ||
| "description": "Centered text on the 'All channels' page saying that there is no content to show." |
There was a problem hiding this comment.
This text isn't necessarily actually true — if the user's not an admin, there might be private channels they don't know about, right?
If that's what web says in a similar situation, though, then whatever. This probably isn't a common situation to actually see.
| } | ||
| } | ||
|
|
||
| class AllChannels extends StatelessWidget { |
There was a problem hiding this comment.
nit: feels a bit odd for this to be public; if it is, it probably needs a more explicit name, like …PageBody
| // TODO(linter): The linter incorrectly flags the following regexp string | ||
| // as invalid. See: https://github.com/dart-lang/sdk/issues/61246 | ||
| // ignore: valid_regexps | ||
| static final _startsWithEmojiRegex = RegExp(r'^\p{Emoji}', unicode: true); | ||
|
|
||
| void _sort(List<ZulipStream> list) { | ||
| list.sort((a, b) { | ||
| // A user gave feedback wanting zulip-flutter to match web in putting | ||
| // emoji-prefixed channels first; see #1202. | ||
| // TODO(#1165) for matching web's ordering completely, which | ||
| // (for the all-channels view) I think just means locale-aware sorting. | ||
| final aStartsWithEmoji = _startsWithEmojiRegex.hasMatch(a.name); | ||
| final bStartsWithEmoji = _startsWithEmojiRegex.hasMatch(b.name); | ||
| if (aStartsWithEmoji && !bStartsWithEmoji) return -1; | ||
| if (!aStartsWithEmoji && bStartsWithEmoji) return 1; | ||
|
|
||
| return a.name.toLowerCase().compareTo(b.name.toLowerCase()); |
There was a problem hiding this comment.
This is duplicating logic that's on the subscribed-channels page, right? If nothing else, let's leave comments pointing both ways between the duplicates.
I think we can do better, though: this comparison function can get factored out to live in model code, say as a static on ChannelStore.
| final sliverList = SliverPadding( | ||
| padding: EdgeInsets.symmetric(vertical: 8), |
There was a problem hiding this comment.
The bottom padding here will be in addition to the minimum 8px bottom padding added by the SliverSafeArea below, right? Is that intended?
| return ConstrainedBox( | ||
| constraints: BoxConstraints(minHeight: 40), |
There was a problem hiding this comment.
Does this constraint have an effect? The "more" button will be height 40 anyway, right?
In general I think the basic reason to have a min height (or min width) like this one is to make an adequate touch target. When that's the reason to have it, the constraint should be kept close to the actual touch recognizer as much as possible — that makes the purpose clear, and also helps ensure it's accomplishing the purpose (rather than making some ancestor big but not the target itself).
| final groupWithSelf = eg.userGroup(members: [eg.selfUser.userId]); | ||
| final groupWithoutSelf = eg.userGroup(members: [eg.otherUser.userId]); | ||
| final groupSettingWithSelf = GroupSettingValueNamed(groupWithSelf.id); | ||
| final groupSettingWithoutSelf = GroupSettingValueNamed(groupWithoutSelf.id); |
There was a problem hiding this comment.
nit: can simplify a bit by cutting out the named groups:
| final groupWithSelf = eg.userGroup(members: [eg.selfUser.userId]); | |
| final groupWithoutSelf = eg.userGroup(members: [eg.otherUser.userId]); | |
| final groupSettingWithSelf = GroupSettingValueNamed(groupWithSelf.id); | |
| final groupSettingWithoutSelf = GroupSettingValueNamed(groupWithoutSelf.id); | |
| final groupSettingWithSelf = GroupSettingValueNameless( | |
| directMembers: [eg.selfUser.userId], directSubgroups: []); | |
| final groupSettingWithoutSelf = GroupSettingValueNameless( | |
| directMembers: [], directSubgroups: []); |
which then don't need store.addUserGroups either.
There was a problem hiding this comment.
That GroupSettingValueNameless constructor is probably also a good candidate to get a terser alias in example_data.dart, with optional and shorter-named parameters.
| await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id, | ||
| navigatorObservers: [transitionDurationObserver], | ||
| child: const HomePage())); | ||
|
|
||
| // global store, per-account store | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| // Switch to channels tab. | ||
| await tester.tap(find.byIcon(ZulipIcons.hash_italic)); | ||
| await tester.pump(); | ||
|
|
||
| if (subscriptions.isEmpty) { | ||
| // expect empty-content placeholder with link | ||
| await tester.tapOnText(find.textRange.ofSubstring('All channels')); | ||
| } else { |
There was a problem hiding this comment.
Hmm — seems like it'd be easier to have TestZulipApp show the desired page directly:
await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
navigatorObservers: [transitionDurationObserver],
child: const AllChannelsPage()));That's what we do in I think most of our similar tests. (E.g. widgets/message_list_test.dart.) Also makes this page's tests less dependent on the details of how we've arranged the navigation through the rest of the app.
It's probably good to have a test or two on these navigation flows themselves, but those can be their own test cases and can skip this setup function.
| // Check that the UI list shows exactly the intended channels, in order. | ||
| // TODO it seems like the list-building optimization (saving resources for | ||
| // offscreen items) would break this if there's much more than a screenful | ||
| // of channels. For expediency we just test with less than a screenful. | ||
| check( | ||
| tester.widgetList(find.byType(AllChannelsListEntry)) | ||
| ).deepEquals( |
There was a problem hiding this comment.
Sure, this solution seems fine. What's the follow-up you have in mind that the TODO refers to?
The comment is useful to have without a TODO marker, though. Particularly in case someone makes a change that pushes it to more than a screenful, and then they're wondering why this test broke.
| tester.widgetList(find.byType(AllChannelsListEntry)) | ||
| ).deepEquals( | ||
| channelsInUiOrder.map<Condition<Object?>>((channel) => | ||
| (it) => it.isA<AllChannelsListEntry>().channel.streamId.equals(channel.streamId)) |
There was a problem hiding this comment.
nit: keep key info within 80 cols
| (it) => it.isA<AllChannelsListEntry>().channel.streamId.equals(channel.streamId)) | |
| (it) => it.isA<AllChannelsListEntry>().channel | |
| .streamId.equals(channel.streamId)) |
(I think the line break helps readability a bit also by splitting the steps.)
| return channel; | ||
| } | ||
|
|
||
| Future<ZulipStream> addPrivateChannelWithoutContentAccess(String? name) async { |
There was a problem hiding this comment.
This is just the default for a private channel, right? (When the user isn't subscribed.)
Fixes #188.
Here is the long-awaited "All channels" page!
See Figma:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=7723-6411&m=dev
Notable features not included here:
Screenshots coming soon.