Add topic-list page #1500
Add topic-list page #1500
Conversation
PIG208
force-pushed
the
pr-topics
branch
2 times, most recently
from
a2d1174 to
e8fb3c3
Compare
PIG208
force-pushed
the
pr-topics
branch
2 times, most recently
from
9464d23 to
1a3d8e9
Compare
|
Updated the PR to implement a intentional deviation from the Figma design on aligning items, discussed in #mobile-team > topic list item alignment @ 💬. |
|
For the "small" and "large" screenshots, I think it would look much better if the checkmarks and indicators on the right scaled too. |
|
Let's name the option "List of topics". I think that's a bit easier to parse, and it's what we're currently testing for the web app UI. |
Seems reasonable. I'd want to clamp the scale factor, for the reasons described at #1023. (For existing examples of implementing that, search for |
|
Thanks both! Updated the PR and the screenshots. I picked 1.5 to be the I clamped only the icons, though, not the topic name text. Because I think #1023 applies to scaling relatively unimportant information. |
|
Works for me, thanks! |
|
The app bar's back button and "Channel feed" button are broken; sometimes when I tap it doesn't respond and this gets logged: Looks like an upstream regression, flutter/flutter#168445, that's being worked on. Edit: Oh, I guess that issue's description says "app crash"—the app isn't crashing for me, but I am seeing "setState() or markNeedsBuild() called when widget tree was locked." like in a stack trace posted there: flutter/flutter#168445 (comment) |
|
We should use less than 28px padding at the start of each topic row. From the issue description:
|
|
For the layout, if we use an empty box as a placeholder for an icon when it's absent, it should scale exactly the same way as the icon does. See the start of the topic text being misaligned in the "large" screenshot: screenshot
|
|
I started a discussion about the chevron-down icon in the app bar: #mobile-team > topic list: chevron-down icon in app bar? @ 💬 |
|
Thanks for the review! I adjusted the padding at the start of each row to 6px (matching the start padding the "pinned" icon would have), and addressed the other issues. The screenshots have been updated.
Thanks for looking into this! Yeah, for this one, we might need flutter/flutter#168546 to land first. |
AFAICT the symptoms of that issue don't reproduce in a release build; the buttons work fine the first time. (I spent a few minutes just now trying to reproduce, in order to evaluate whether the issue will affect the upcoming beta v0.0.29.) In a debug build, I also see it on other screens even without this PR. So definitely an annoyance in dev — plus might be a sign of further bugginess that's just harder to spot the symptoms of — and will be good to see fixed upstream. But I don't think it needs to interact with merging this PR. |
|
When I rebase this onto current Could you rebase and fix those please? |
|
Thanks! Just updated this. I found that it also happens to the last commit. All commits that contain changes adding new strings to translate are probably affected, since we just recently added |
There was a problem hiding this comment.
Thanks, this is exciting!! Small comments below, and it looks like there's a (nearly?) resolved discussion about making this page more accessible from a message-list page for a topic narrow: #mobile > Flutter beta: missing topic list @ 💬
| final appBarBackgroundColor = colorSwatchFor( | ||
| context, store.subscriptions[streamId]).barBackground; |
There was a problem hiding this comment.
nit: I would put context, on the previous line
lib/widgets/topic_list.dart
Outdated
| shape: Border(bottom: BorderSide( | ||
| width: 1, color: designVariables.borderBar))), |
There was a problem hiding this comment.
nit: omit shape; the same shape is set by zulipThemeData
| for (final GetStreamTopicsEntry(:maxId, name: topic) in lastFetchedTopics!) { | ||
| final unreadMessageIds = | ||
| unreadsModel!.streams[widget.streamId]?[topic] ?? <int>[]; | ||
| final countInTopic = unreadMessageIds.length; |
There was a problem hiding this comment.
Let's centralize on Unreads.countInTopicNarrow for this; I could imagine wanting to refine it for something like muted senders etc.
There was a problem hiding this comment.
We might still need to keep unreadMessageIds (or inline it) to compute the value of hasMention.
Looks like inbox page does something like that with DMs:
final countInNarrow = unreadsModel!.countInDmNarrow(dmNarrow);
if (countInNarrow == 0) {
continue;
}
final hasMention = unreadsModel!.dms[dmNarrow]!.any(
(messageId) => unreadsModel!.mentions.contains(messageId));There was a problem hiding this comment.
Makes sense; yeah, my comment was specifically about using .length; it's fine to do other things besides that.
lib/widgets/topic_list.dart
Outdated
| // A null [Icon.icon] makes a blank space. | ||
| _IconMarker(icon: topic.isResolved ? ZulipIcons.check : null), | ||
| Expanded(child: Opacity( | ||
| opacity: isTopicVisibleInStream ? 1 : 0.5, |
There was a problem hiding this comment.
This looks a little odd—the value (from the relevant dartdoc) is about "whether" this topic should appear, not how it should appear. Is this logic correct, and is that dartdoc on store.isTopicVisibleInStream correct?
There was a problem hiding this comment.
The topics with "isTopicVisibleInStream=false" will be hidden on web, in the list that appears when you just click on the channel.
This topic-list however should probably match the topic-list from "show all topics" button on web. Because in the Figma design, muted topics in the topic-list appear semi-transparent. I think similar to web, this offers a way for the user to access the muted topics.
In conclusion, both are correct, but we should probably name the bool differently and comment on why we use the helper this way.
There was a problem hiding this comment.
Yeah, that value means whether it should appear in the channel feed, the message list. That'd be good to clarify in its doc.
Here in the list of topics for the channel, all topics are shown.
There was a problem hiding this comment.
Or maybe it is more appropriate to use topicVisibilityPolicy directly, and compare the value to UserTopicVisibilityPolicy.muted.
There was a problem hiding this comment.
Yeah, that's probably a good way to write this code.
lib/widgets/topic_list.dart
Outdated
| if (trailingWidgets.isEmpty) | ||
| const SizedBox(width: 53), |
There was a problem hiding this comment.
I see this is what the Figma is doing, but it feels odd: it doesn't create a consistent end margin for the topic text, because the gap will be less than 53px when there's just one or two trailingWidgets, right? How about just removing this and allowing a longer line for the topic text.
lib/widgets/topic_list.dart
Outdated
| // This is adapted from [UnreadCountBadge]. | ||
| class _UnreadCountBadge extends StatelessWidget { | ||
| const _UnreadCountBadge({required this.count}); |
There was a problem hiding this comment.
It really seems like we should be able to use UnreadCountBadge directly here—if the Figma has inconsistencies among unread-count badges, I'd suspect they're accidental; could you investigate?
There was a problem hiding this comment.
Yeah, it would be ideal to share it.
The UnreadCountBadge we know today comes from Nov 2023 (d9eb0d3), when it was extracted from RecentDmConversationsPage. For DM, the current Figma design's unread count badge looks different.
I assume that the design for inbox page and subscription-list page got outdated this way too. Since we haven't got to implementing those redesigns yet, just updating UnreadCountBadge to use the new design might make it out-of-place in those pages.
Relatedly, if we can, I would like to extract _TopicItem from lib/widgets/inbox.dart for reuse too. When I tried that while drafting this, it turned out that we would need to implement more of the inbox-page redesign to make the shared widget fit.
I think a reasonable strategy will be implementing these widgets in topic-list first, while referring to the code structure of the existing ones (to compare what's changed). We can consider extracting these widgets once we get to implementing other redesigned pages, and phase out UnreadCountBadge in this process.
There was a problem hiding this comment.
Sure, that sounds reasonable. Could you leave a TODO for this plan?
There was a problem hiding this comment.
Yeah, I plan to file some issues later.
test/widgets/topic_list_tests.dart
Outdated
| await tester.pumpWidget(TestZulipApp( | ||
| accountId: eg.selfAccount.id, | ||
| child: TopicListPage(streamId: channel.streamId))); | ||
| await tester.pumpAndSettle(); |
There was a problem hiding this comment.
Can this be done with individual tester.pumps rather than a pumpAndSettle?
test/widgets/topic_list_tests.dart
Outdated
| assert(resolvedTopic.displayName == '✔ resolved', resolvedTopic.displayName); | ||
| check(findInTopicItemAt(0, find.text('✔ resovled'))).findsNothing(); |
There was a problem hiding this comment.
oops: should be find.text('✔ resolved')
|
Thanks! Updated the PR. |
|
Thanks, looks good! Marking for Greg's review. |
chrisbobbe
added
integration review
There was a problem hiding this comment.
Thanks to you both! Generally this looks good; comments below.
I'm about to go AFK, but I've gotten up through the non-test changes in the second commit:
39b6724 topics: Add topic list page
Still ahead for me to read are the tests, and the third commit:
33fc01d topics: Add TopicListButton to channel action sheet
| context, store.subscriptions[streamId]).barBackground; | ||
|
|
||
| return PageRoot(child: Scaffold( | ||
| appBar: ZulipAppBar( |
There was a problem hiding this comment.
topics: Add topic list page
For the topic-list page app bar, we leave out the icon "chveron_down.svg"
nit: spelling of icon name
| import 'text.dart'; | ||
| import 'theme.dart'; | ||
|
|
||
| class TopicListPage extends StatelessWidget { |
There was a problem hiding this comment.
The topic-list implementation is quite similar to parts of inbox page
and message-list page. Therefore, we structure the code to make them
look similar to compare for changes side-by-side to help with reviewing
what has changed.
That's true, but FTR there's another reason this is useful, which in my mind is actually a bigger reason in this case: it helps with comparing them in the future, for maintaining them.
Specifically it helps us (a) when we're changing one, apply the same change to the other, where appropriate, and (b) later reconcile the differences they do have and then refactor to unify them.
lib/widgets/message_list.dart
Outdated
| }, | ||
| behavior: HitTestBehavior.opaque, | ||
| child: Padding( | ||
| padding: EdgeInsetsDirectional.fromSTEB(4, 8, 12, 8), |
There was a problem hiding this comment.
Is there an example in Figma with both the feed icon and this action? They look crowded together in the UI:
So this should probably get more padding at the start. I'd be inclined to make it symmetric — that's how the icons are.
(I see it shows 4px padding in Figma… but that's in a situation where the amount of start padding has basically no visible effect, because the channel name is way off to the left anyway. If the channel name were so long that it bumped into this padding, I suspect 4px would look too crowded then too.)
There was a problem hiding this comment.
Went with 12px, and posted a screenshot in #mobile > Flutter beta: missing topic list @ 💬:
lib/widgets/topic_list.dart
Outdated
| final int streamId; | ||
| final bool willCenterTitle; | ||
|
|
||
| Widget _buildAppBarRow(BuildContext context) { |
There was a problem hiding this comment.
| Widget _buildAppBarRow(BuildContext context) { | |
| Widget _buildTitleRow(BuildContext context) { |
This whole widget is only part of the app bar, namely the title. So the widget this helper returns can be at most the title.
There was a problem hiding this comment.
In fact better:
| Widget _buildAppBarRow(BuildContext context) { | |
| Widget _buildStreamRow(BuildContext context) { |
This is really doing the exact same job as the _buildStreamRow on MessageListAppBarTitle — just for the updated design. So matching the name helps draw that parallel.
lib/widgets/topic_list.dart
Outdated
| MessageListPage.buildRoute(context: context, | ||
| narrow: ChannelNarrow(streamId)))), | ||
| ], | ||
| backgroundColor: appBarBackgroundColor), |
There was a problem hiding this comment.
nit: put this above title and actions; the latter are more the content of the app bar (similar to a child argument, which always goes last), and this is more like metadata
lib/widgets/topic_list.dart
Outdated
|
|
||
| return ListView.builder( | ||
| itemCount: topicItems.length, | ||
| itemBuilder: (BuildContext context, int index) => |
There was a problem hiding this comment.
nit: omit boring types
| itemBuilder: (BuildContext context, int index) => | |
| itemBuilder: (context, index) => |
lib/widgets/topic_list.dart
Outdated
| final opacity = | ||
| visibilityPolicy == UserTopicVisibilityPolicy.muted ? 0.5 : 1.0; |
There was a problem hiding this comment.
Let's use a switch-expression for this — helps give confidence we've considered all the cases, particularly if we ever add another value to this enum.
lib/widgets/topic_list.dart
Outdated
| // To account for scaled text, we align everything on the row | ||
| // to [CrossAxisAlignment.center] instead ([Row]'s default), | ||
| // like we do for the topic items on the inbox page. | ||
| // CZO discussion: |
There was a problem hiding this comment.
There's one quite visible thing we're giving up from the design in this version, so let's note it in a TODO:
| // To account for scaled text, we align everything on the row | |
| // to [CrossAxisAlignment.center] instead ([Row]'s default), | |
| // like we do for the topic items on the inbox page. | |
| // CZO discussion: | |
| // To account for scaled text, we align everything on the row | |
| // to [CrossAxisAlignment.center] instead ([Row]'s default), | |
| // like we do for the topic items on the inbox page. | |
| // TODO(design): align to baseline (and therefore to first line of | |
| // topic name), but with adjustment for icons | |
| // CZO discussion: |
| // https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/topic.20list.20item.20alignment/near/2173252 | ||
| children: [ | ||
| // A null [Icon.icon] makes a blank space. | ||
| _IconMarker(icon: topic.isResolved ? ZulipIcons.check : null), |
There was a problem hiding this comment.
Should this get the opacity applied too?
There was a problem hiding this comment.
No, because it's not extra-faded in the Figma design. (This is also why we couldn't wrap the entire row in a single Opacity widget.)
| fontStyle: topic.displayName == null ? FontStyle.italic : null, | ||
| color: designVariables.textMessage, | ||
| ), | ||
| topic.unresolve().displayName ?? store.realmEmptyTopicDisplayName))), |
There was a problem hiding this comment.
maxLines: 3? I don't see it come up in the Figma design but I believe that's the intent.
(And overflow ellipsis.)
There was a problem hiding this comment.
Hm, I thought the example text "[…] which could be more than 2 lines if we want" meant that we can have >2 lines with no upperbound. But I guess without baseline alignment, more than 3 lines can look odd. Let's go with maxLines: 3 now and revisit this if we fix the alignment.
There was a problem hiding this comment.
Opened #1528 for implementing baseline alignment.
PIG208
force-pushed
the
pr-topics
branch
2 times, most recently
from
263f453 to
429fe40
Compare
test/widgets/action_sheet_test.dart
Outdated
| checkButton('Mark channel as read'); | ||
| checkButton('List of topics'); |
There was a problem hiding this comment.
nit: match the order these appear in the action sheet (and in the code, and the tests below)
| checkButton('Mark channel as read'); | |
| checkButton('List of topics'); | |
| checkButton('List of topics'); | |
| checkButton('Mark channel as read'); |
test/widgets/topic_list_tests.dart
Outdated
| import '../stdlib_checks.dart'; | ||
| import 'test_app.dart'; | ||
|
|
||
| void main() { |
There was a problem hiding this comment.
Filename should end in _test.dart so that flutter test picks it up 🙂
+++ test/widgets/topic_list_tests.dart
test/widgets/topic_list_tests.dart
Outdated
| eg.getStreamTopicsEntry(name: resolvedTopic.apiName), | ||
| eg.getStreamTopicsEntry(name: unresolvedTopic.apiName), | ||
| ]); | ||
|
|
||
| assert(resolvedTopic.displayName == '✔ resolved', resolvedTopic.displayName); | ||
| check(findInTopicItemAt(0, find.text('✔ resolved'))).findsNothing(); |
There was a problem hiding this comment.
Several of these test cases are a bit fragile because they let maxId be the same for several items in the list (which is also unrealistic — the message with that ID can belong to at most one conversation), and then their checks expect a particular order for the items to appear in.
Can fix by just giving explicit maxId: 2, maxId: 1, etc.
test/widgets/topic_list_tests.dart
Outdated
| check(find.text('topic B')).findsOne(); | ||
| }); | ||
|
|
||
| group('_TopicItem', () { |
There was a problem hiding this comment.
nit: this group isn't really specific to _TopicItem — a lot of the logic it's testing is in _TopicListState. (In fact I saw this group and that there weren't any other tests left in the file, and then thought that meant there wouldn't be tests for what's in _TopicListState.build and wondered if I should ask for some.)
I think the group can just be unwrapped, and its contents de-indented to the top level of main. One accurate name for the group would be "page body"… but the whole file is about this particular page, so it seems reasonable for the page body to take the top level.
For the topic-list page app bar, we leave out the icon "chevron_down.svg" since it's related to a new design (zulip#1039) we haven't implemented yet. This also why "TOPICS" is not aligned to the middle part of the app bar on the message-list page. We also leave out the new topic button and topic filtering, which are out-of-scope for zulip#1158. The topic-list implementation is quite similar to parts of inbox page and message-list page. Therefore, we structure the code to make it easy to maintain in the future. Especially, this helps us (a) when we're changing one, apply the same change to the other, where appropriate, and (b) later reconcile the differences they do have and then refactor to unify them. Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6819-35869&m=dev The "TOPICS" icon on message-list page in a topic narrow is a UX change from the design. See CZO discussion: https://chat.zulip.org/#narrow/channel/48-mobile/topic/Flutter.20beta.3A.20missing.20topic.20list/near/2177505
The icon was taken from CZO discussion: https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/Topic.20list.20in.20channel/near/2140324 Fixes: zulip#1158 Co-authored-by: Zixuan James Li <zixuan@zulip.com>
Stacked on top of #1491.
Some non-goals of this change are deferred to #1499. In this implementation, we fetch the topics but do not handle all events to receive live-updates. It's expected that when topics are resolved/unresolved or moved, or when new messages arrived, the changes to the topic-list will not be seen until the next fetch.
We also skip thinning down the app bar, since that will require work on app bars on message-list page too.
The PR is structured to encourage side-by-side comparison with similar existing code. Namely
_TopicItemfromlib/widgets/inbox.dartandMessageListAppBarTitle.Fixes: #1158
screenshots (taken on my Android device, hence the left-aligned app bar!)
debugDefaultTargetPlatformOverride = TargetPlatform.iOS: