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! |
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