Use channel folders in the "Inbox" view#2186
Conversation
Mmm, let's make sure to discuss once more in #mobile-design before you do that work, since it's been a while since the design was proposed, to make sure we're still happy with all the ideas. Moving the buttons to collapse/expand channels to the right will be nice for getting more space for topic names (less left-side margin), and matching web better. |
|
Left a comment on the extra folder name commit at #mobile-design > channel folders in inbox: sticky headers @ 💬 (I'm worried it won't work well). |
|
I think the top DM row should be vertically closer to its "folder" label. Currently, it looks to be equidistant, which I don't think feels right semantically. |
|
I guess folders aren't collapsible at this point? We should probably track making them collapsible as a follow-up. |
|
Overall, the screenshots look reasonable to me -- I'm excited for this feature! |
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe! Sorry for the delay! All LGTM, moving over to Greg's review.
gnprice
left a comment
There was a problem hiding this comment.
Thanks for building this! Generally this looks great; mostly small comments below.
The two parts I haven't read are the last commit (mentioned below) and the tests of the main, second-to-last commit:
e601570 inbox: Group channels by folder, including realm-level folders
| return Semantics(container: true, | ||
| child: result); |
There was a problem hiding this comment.
This Semantics widget seems to disappear from InboxChannelHeaderItem in the _HeaderItem refactor:
3d92f97 inbox [nfc]: Merge _HeaderItem base class into its only remaining subclass
A second one appears in an earlier commit on InboxFolderHeaderItem. Is it intended to move from one to the other, or should it be on both?
There was a problem hiding this comment.
Ah sounds like I misresolved a conflict with #2120 :) thanks for spotting!
There was a problem hiding this comment.
(It should be on both.)
| final DmNarrow narrow; | ||
| final int count; | ||
| final bool hasMention; | ||
| final List<(DmNarrow, int, bool)> items; |
There was a problem hiding this comment.
The count and the mention icon effectively get removed from the DMs header in this commit:
a6bb747 inbox: Use folder-style header for all-DMs, too
I see in the screenshots that's intentional, and I guess it's implied by "folder-style" since we don't put those on folders. Would be good to briefly mention in the commit message.
| case _InboxListItemDmConversation(:final narrow, :final count, :final hasMention): | ||
| return InboxDmItem(narrow: narrow, count: count, hasMention: hasMention); |
There was a problem hiding this comment.
nit: This way is fine, but it can make things a bit simpler to pass the whole item object down instead of each of its fields separately:
| case _InboxListItemDmConversation(:final narrow, :final count, :final hasMention): | |
| return InboxDmItem(narrow: narrow, count: count, hasMention: hasMention); | |
| case _InboxListItemDmConversation(): | |
| return InboxDmItem(data: item); |
In particular that makes fewer places that need to be updated when we add or alter a field on the data class.
(This is the same pattern seen in our content widgets, which generally take a whole node object of some relevant subclass of ContentNode.)
There was a problem hiding this comment.
Ah yeah, agreed. I'd like to defer this for now if that's OK.
| Finder findChannelHeader(int channelId) => find.byWidgetPredicate((widget) => | ||
| widget is InboxChannelHeaderItem && widget.channelId == channelId).first; | ||
| widget is InboxChannelHeaderItem && widget.subscription.streamId == channelId).first; |
assets/l10n/app_en.arb
Outdated
| "unknownChannelFolderName": "(unknown channel folder)", | ||
| "@unknownChannelFolderName": { | ||
| "description": "Name placeholder to use for a channel folder when we don't know its name." |
There was a problem hiding this comment.
Huh, can this situation happen?
test/model/channel_test.dart
Outdated
| final sorted = unsorted.toList() | ||
| .sorted((a, b) => store.compareUiChannelFolders(a, b)); |
There was a problem hiding this comment.
nit: simplify a bit with a tearoff
| final sorted = unsorted.toList() | |
| .sorted((a, b) => store.compareUiChannelFolders(a, b)); | |
| final sorted = unsorted.toList() | |
| .sorted(store.compareUiChannelFolders); |
(which might then fit on one line?)
lib/widgets/inbox.dart
Outdated
| final channelSections = channelSectionsByFolder[folder]!.toList(); | ||
| channelSections.sort((a, b) { |
There was a problem hiding this comment.
No need for the toList() copy; we can sort the existing list. (Just like we sort the pinnedChannelSections and otherChannelSections lists as of the commit before this one.)
| final uiChannelFolder = store.uiChannelFolder(streamId); | ||
| (channelSectionsByFolder[uiChannelFolder] ??= []) |
There was a problem hiding this comment.
There's a user setting to control whether folders apply to their inbox, right? Do we want to implement that setting before we ship this?
Probably isn't hard to add.
There was a problem hiding this comment.
Yyyes, but it's marked as "web/desktop" in the API doc:
https://zulip.com/api/register-queue
web_inbox_show_channel_folders: boolean
Determines whether channel folders are used to organize how conversations with unread messages are displayed in the web/desktop application's Inbox view.
Changes: New in Zulip 12.0 (feature level 431).
lib/widgets/inbox.dart
Outdated
| // TODO(design) this is kind of a hack; we'd really like to keep the | ||
| // folder name onscreen by making it sticky too, i.e. make | ||
| // both folder headers and channel headers sticky headers. | ||
| // See discussion for defining and implementing that behavior: | ||
| // https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/channel.20folders.20in.20inbox.3A.20sticky.20headers/near/2367795 | ||
| final bool showChannelFolderName; |
There was a problem hiding this comment.
I'm skipping reading the code in this last commit:
7f1ada4 inbox: Show channel-folder name in channel header, only in "sticky" position
because it sounds like Alya's view above is that we don't want it product/design-wise.
test/widgets/inbox_test.dart
Outdated
| testWidgets('only pinned channels: shows pinned header, no other header', (tester) async { | ||
| final channel = eg.stream(); | ||
| await setupPage(tester, | ||
| streams: [channel], | ||
| subscriptions: [eg.subscription(channel, pinToTop: true)], | ||
| unreadMessages: [eg.streamMessage(stream: channel)]); | ||
| checkFolderHeader('Pinned channels'); | ||
| checkNoFolderHeader('Other channels'); | ||
| }); | ||
|
|
||
| testWidgets('only unpinned channels: shows other header, no pinned header', (tester) async { |
There was a problem hiding this comment.
These two test cases would be sharper if in each of them a channel in the other situation existed but didn't have any messages. That shows the logic for which headers to show is based on the (unread) messages appearing in the inbox, not on the full roster of channels.
This is equivalent except that it puts channels that start with an emoji first; see implementation comment in the method. Test written with help from Claude. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This more generic semantics can accommodate list items that aren't whole sections, such as folder headers (coming soon) and eventually topic items, for zulip#389 .
Tests written with Claude. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This removes the unread count and @-mention icon from the all-DMs header, which the channel-folder headers will also leave out for now.
…class This _HeaderItem class had two subclasses before: (a) one for the all-DMs section header, InboxAllDmsHeaderItem (b) one for channel section headers, InboxChannelHeaderItem We recently restyled the all-DMs section header as a folder-style header, dropping (a). Now, simplify InboxChannelHeaderItem by having it absorb all that _HeaderItem was doing.
Clients are actually expected to show "PINNED" and "OTHER" sections in the Inbox, with the same styling as channel folders that admins can create for the org with Zulip Server 11+. We can think of these two entities as "pseudo" channel folders. "UI channel folder" seems like a reasonable umbrella term for these "pseudo" channel folders and the realm/org-level channel folders. Also replace the existing compareChannelFolders function, which acted only on realm-level folders, with a new function compareUiChannelFolders that's actually what we'll want for the inbox. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes zulip#1765. Tests written with help from Claude. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7f1ada4 to
0b772ba
Compare
Fixes #1765.
cc @alya
Note on the screenshots: I think the view looks "busy" because the channel-header and unread-marker backgrounds are so bold. I'd like to follow up with a PR to finish updating the "Inbox" design to follow Figma, which will make these elements more subtle:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=13188-51396&m=dev
Screenshots
As discussed at #mobile-design > channel folders in inbox: sticky headers @ 💬, here's a proposal for showing the channel folder in a channel header's name, but only in the sticky-header position. As discussed, we'd ideally make folder headers and channel headers both be sticky headers, but we haven't really defined what that means, and it's probably nontrivial to implement.
We also discussed that this part isn't crucial, so I've made a separate commit for it that we can drop if desired.
Screenshots of this detail: