topicList: Add topic list page for each channel #1449
Conversation
lakshya1goel
force-pushed
the
issue1158
branch
3 times, most recently
from
dfc6f79 to
6f6e5b6
Compare
lakshya1goel
force-pushed
the
issue1158
branch
2 times, most recently
from
5dea5f1 to
5a7871f
Compare
|
Why are the topics italicized? We generally use italics only for the "general chat" topic. |
|
Hi @PIG208 I have pushed the revision and also updated the PR description. Please have a look. Thanks! |
There was a problem hiding this comment.
Thanks! I did a partial review (didn't go through the design/implementation thoroughly), and left out the tests for this round. There are multiple changes needed for this to match the design.
When working on the next revision, please make sure that you self-review your PR carefully, since a lot of these issues can be caught with this process.
lib/widgets/topic_list.dart
Outdated
| final hasUnreads = unreads > 0; | ||
|
|
||
| return Material( | ||
| type: MaterialType.transparency, |
There was a problem hiding this comment.
Why type: MaterialType.transparency? Looks like the design uses bg-message-regular for background color.
lib/widgets/topic_list.dart
Outdated
| if (hasUnreads) ...[ | ||
| UnreadCountBadge( | ||
| count: unreads, | ||
| backgroundColor: designVariables.bgCounterUnread, | ||
| bold: true, | ||
| ), | ||
| ], |
There was a problem hiding this comment.
Looks like this only handle the marker for unreads. Like _TopicItem for inbox, we should handle all the different markers that might appear here.
lib/widgets/topic_list.dart
Outdated
| ], | ||
| ), | ||
| ), | ||
| ), | ||
| ]), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Wrap closing )'s, when possible, like we do in other places.
lib/widgets/topic_list.dart
Outdated
| topic.name.displayName.startsWith('✔') | ||
| ? topic.name.displayName.substring(2).trim() | ||
| : topic.name.displayName, |
There was a problem hiding this comment.
We can just use topic.name.unresolve() instead.
lib/widgets/topic_list.dart
Outdated
| child: SizedBox( | ||
| height: 16, | ||
| width: 16, | ||
| child: topic.name.displayName.startsWith('✔') |
lib/widgets/topic_list.dart
Outdated
| Future<void> _fetchTopics() async { | ||
| try { | ||
| await _model!.fetchTopics(); | ||
| if (_model!.hasError && mounted) { | ||
| final zulipLocalizations = ZulipLocalizations.of(context); | ||
| showErrorDialog( | ||
| context: context, | ||
| title: zulipLocalizations.errorFetchingTopics, | ||
| message: _model!.errorMessage); | ||
| } | ||
| } catch (e) { | ||
| if (!mounted) return; | ||
| final zulipLocalizations = ZulipLocalizations.of(context); | ||
| showErrorDialog( | ||
| context: context, | ||
| title: zulipLocalizations.errorFetchingTopics, | ||
| message: e.toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think with the way how fetchTopics is used right now, having a separate class for the data does not feel like a helpful abstraction, since it adds a layer of indirection as a trade-off.
Right now, we only fetch topics onNewStore, and there is no retry logic yet. For error handling, The _fetchTopics helper does not really care if errorMessage lives on as a part of the model. This also requires as to manage an additional listener of a ChangeNotifier.
It would probably be cleaner to inline the getStreamTopics here, and only consider extracting a model when there is more complexity that warrants a thicker data model. The only mutable states needed are isLoading and topics.
lib/widgets/topic_list.dart
Outdated
| ? Icon( | ||
| ZulipIcons.check, | ||
| size: 12, | ||
| color: designVariables.textMessage, | ||
| ) |
There was a problem hiding this comment.
The opacity of the icon does not match the design:
There was a problem hiding this comment.
The size of the icon should be 16px.
lib/widgets/topic_list.dart
Outdated
| child: Row(crossAxisAlignment: CrossAxisAlignment.start, children: [ | ||
| const SizedBox(width: 28), | ||
| Padding( | ||
| padding: const EdgeInsets.only(top: 10.0), |
There was a problem hiding this comment.
The bottom padding of the icon is missing.
lib/widgets/topic_list.dart
Outdated
| bold: true, | ||
| ), | ||
| ], | ||
| ], |
There was a problem hiding this comment.
There is 12px padding before the end:
lib/widgets/topic_list.dart
Outdated
| context, | ||
| channelId: streamId, | ||
| topic: topic.name, | ||
| someMessageIdInTopic: null, |
There was a problem hiding this comment.
Looks like we can conveniently use topic.maxId for this.
|
Hi @PIG208, thanks for the review. I have pushed the update. PTAL. Thanks! |
There was a problem hiding this comment.
Thanks for the update! I went through the implementation and took a look at the tests. Left some more comments.
I think the various markers (unreads, mentions, topic visibility) are non-trivial to implement, but that do share a lot of commonality with inbox topic items, so I wonder if it's better to figure out a way to share code, at least for the markers, between the topic list items and inbox topic items. @gnprice might have thoughts on this.
An advantage of that would be not needing to maintain a separate set of tests for a very similar feature.
lib/widgets/topic_list.dart
Outdated
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| Expanded( | ||
| child: Text(topic.name.unresolve().toString(), |
There was a problem hiding this comment.
We should avoid using toString on TopicName. Instead, use TopicName.displayName whenever a topic appears in user-facing text.
lib/widgets/topic_list.dart
Outdated
| color: designVariables.textMessage, | ||
| ), | ||
| ) | ||
| : null, |
There was a problem hiding this comment.
nit: How about replacing this with the outer SizedBox? Icon doesn't need the SizedBox since we can just set its size.
There was a problem hiding this comment.
Wouldn't removing the SizedBox around the resolved checkmark icon cause layout misalignment between resolved and unresolved topics?
There was a problem hiding this comment.
Oh yeah. I meant that we can have either the SizedBox or the Icon. We shouldn't need to wrap the Icon inside a SizedBox.
lib/widgets/topic_list.dart
Outdated
| const SizedBox(width: 28), | ||
| Padding( | ||
| padding: const EdgeInsets.symmetric(vertical: 10.0), | ||
| child: SizedBox( |
There was a problem hiding this comment.
nit: we can use SizedBox.square() here
lib/widgets/topic_list.dart
Outdated
| child: UnreadCountBadge(count: unreads, | ||
| backgroundColor: designVariables.bgCounterUnread, bold: true), | ||
| ), | ||
| const SizedBox(width: 8), |
There was a problem hiding this comment.
Hmm, what is this for? The design does call for a always-present 12px trailing padding, but this (combined with the possible unread marker or not) wouldn't match that.
There was a problem hiding this comment.
We have 8px padding between text and markers thus added the 1st 8px width Sizedbox after the text widget.
Then we have 4px gap between any 2 markers thus added 4px end padding for each marker and at last added this 8px sizedbox so that when any of the marker is there the total padding will be 4px + 8px = 12px.
But yes when there is no marker in that case we are having 8xp + 8px = 16px
lib/widgets/topic_list.dart
Outdated
| if (hasMention) const _IconMarker(icon: ZulipIcons.at_sign), | ||
| if (visibilityIcon != null) _IconMarker(icon: visibilityIcon), | ||
| if (hasUnreads) Padding( | ||
| padding: const EdgeInsetsDirectional.only(end: 4), | ||
| child: UnreadCountBadge(count: unreads, | ||
| backgroundColor: designVariables.bgCounterUnread, bold: true), | ||
| ), |
There was a problem hiding this comment.
It feels a bit involved that each of these widget has its own trailing padding. How about wrapping them in a Row with spacing set to 4?
test/widgets/topic_list_test.dart
Outdated
| void main() { | ||
| TestZulipBinding.ensureInitialized(); | ||
|
|
||
| Widget? findRowByLabel(WidgetTester tester, String label) { |
There was a problem hiding this comment.
nit: reserve the verb "find" for helpers that return a Finder; this can be instead called "rowWidgetByLabel`
test/widgets/topic_list_test.dart
Outdated
| Widget? findRowByLabel(WidgetTester tester, String label) { | ||
| final textWidgets = tester.widgetList<Text>(find.text(label)); | ||
| if (textWidgets.isEmpty) return null; | ||
|
|
||
| final rows = tester.widgetList<Row>( | ||
| find.descendant( | ||
| of: find.byType(TopicListItem), | ||
| matching: find.byType(Row), | ||
| )); | ||
|
|
||
| for (final row in rows) { | ||
| if (tester.widgetList(find.descendant( | ||
| of: find.byWidget(row), | ||
| matching: find.text(label), | ||
| )).isNotEmpty) { | ||
| return row; | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Given that we are always using find.byWidget on the result of this, how about just returning a finder?
| Widget? findRowByLabel(WidgetTester tester, String label) { | |
| final textWidgets = tester.widgetList<Text>(find.text(label)); | |
| if (textWidgets.isEmpty) return null; | |
| final rows = tester.widgetList<Row>( | |
| find.descendant( | |
| of: find.byType(TopicListItem), | |
| matching: find.byType(Row), | |
| )); | |
| for (final row in rows) { | |
| if (tester.widgetList(find.descendant( | |
| of: find.byWidget(row), | |
| matching: find.text(label), | |
| )).isNotEmpty) { | |
| return row; | |
| } | |
| } | |
| return null; | |
| } | |
| Finder findRowByLabel(WidgetTester tester, String label) { | |
| return find.descendant( | |
| of: find.byType(TopicListItem), | |
| matching: find.widgetWithText(Row, label)); | |
| } |
test/widgets/topic_list_test.dart
Outdated
| }); | ||
|
|
||
| bool hasIcon(WidgetTester tester, { | ||
| required Widget? parent, |
There was a problem hiding this comment.
It might cleaner to just pass label, since parent is always the result from findRowByLabel.
test/widgets/topic_list_test.dart
Outdated
| return tester.widgetList(find.descendant( | ||
| of: find.byWidget(parent!), | ||
| matching: find.byIcon(icon), | ||
| )).isNotEmpty; |
There was a problem hiding this comment.
It seems a bit unnecessary to convert the finder result to a bool. Let's just return the finder from this helper (we should then rename this too), and use check(…).findsOne etc on the result as appropriate.
This way we have more information about the find result when the check fails.
| )).isNotEmpty; | ||
| } | ||
|
|
||
| group('mentions', () { |
There was a problem hiding this comment.
Another interesting case for this group would be topic with a mention that is not unread.
|
Since we have added the markers in the new revision, let's also include screenshots in the next update. |
Yeah, this TopicListItem class clearly has a lot in common with the _TopicItem in the inbox. At a minimum we should keep the code here as similar as possible to that code, so that it only differs in ways that are deliberately intended because they mean real differences in the user-facing behavior. That way we don't put up unnecessary barriers to ourselves for trying to unify the code later. After making that revision to isolate the intentional differences, it'd be good to post a list of them, and then we can see which ones we want as differences between the two screens and which ones would be good changes to just make to the inbox too. For example it looks like one difference is that this one presents resolved topics with an icon — that's something that would be a good change for the inbox too. I think probably in the end what we'll want is to have
|
|
Hi @lakshya1goel! Do you have any unpushed changes to this draft? Since quite a bit of rewrite is still required to get this merged, we might be taking over this PR to prioritize finishing #1158 sooner, given that it is a launch-blocker issue. |
|
Hey @PIG208, I have pushed the revision. Please have a look. |
|
Thanks @lakshya1goel for your work on this issue. I think we'll be able to get this to something we're happy to merge much more quickly by having one of us on the core team drive it, so I've asked @PIG208 to take on the issue. I believe @PIG208 will be sending a PR for this soon — once that's posted, I recommend you take a look at it and compare. |
|
Thank you for your contribution! We will be closing this, but the work will continue in #1500. |
fixes: #1158
Related CZO Discussion: #mobile-design > Topic list in channel
Figma link: Link
Light Theme:
Dark Theme:
Video Demonstration:
WhatsApp.Video.2025-04-02.at.9.59.45.PM.mp4