topic_list: Add empty state placeholder for channels with no topics #2027
Conversation
There was a problem hiding this comment.
Thanks @khanak0509 for picking this up. Comments below.
Please also provide screenshot(s) of the new changes in the PR description; that way, the reviewers can quickly confirm the visuals. Additionally, these changes need tests.
assets/l10n/app_en.arb
Outdated
| "@zulipAppTitle": { | ||
| "description": "The name of Zulip. This should be either 'Zulip' or a transliteration." | ||
| }, | ||
| "topicListEmptyPlaceholderHeader": "There are no topics here yet", |
There was a problem hiding this comment.
nit:
| "topicListEmptyPlaceholderHeader": "There are no topics here yet", | |
| "topicListEmptyPlaceholderHeader": "There are no topics here yet.", |
assets/l10n/app_en.arb
Outdated
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
No need for the additional lines, please remove.
lib/widgets/topic_list.dart
Outdated
| if (lastFetchedTopics!.isEmpty) { | ||
| final zulipLocalizations = ZulipLocalizations.of(context); | ||
| return PageBodyEmptyContentPlaceholder( | ||
| header: zulipLocalizations.topicListEmptyPlaceholderHeader, | ||
| message: zulipLocalizations.topicListEmptyPlaceholderMessage); | ||
| } |
There was a problem hiding this comment.
The code inside the if block needs to be indented.
|
|
And regarding the screenshots of the new changes ,how should I proceed? |
|
By tests, I meant to write at least one test case in And for the screenshot, you can always mess with the code and mimic the scenario; for example in this case, you can write |
|
|
I have now added a test case (16) in test/widgets/topic_list_test.dart to verify the latest changes, and I’ve also attached a screenshot. |
|
Thanks @khanak0509, and thanks @sm-sayedi for offering a review! From a quick look at the screenshot, the text doesn't match Alya's specification in #2003 (comment); please fix that (delete the second line of text). |
|
|
All changes done and the second line removed. |
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! Small comments below.
Please also squash your fixup commits into the main commit that you're proposing to be merged.
assets/l10n/app_en.arb
Outdated
| }, | ||
| "topicListEmptyPlaceholderHeader": "There are no topics here yet.", | ||
| "@topicListEmptyPlaceholderHeader": { | ||
| "description": "Header text shown when a channel has no topics." | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Follow indentation/newline patterns of existing code
|
|
||
| void main() { | ||
| TestZulipBinding.ensureInitialized(); | ||
|
|
There was a problem hiding this comment.
nit: keep empty line, separating stanzas of code
|
sorry for these mistakes , now is it ok ? |
|
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! And bump on this part of my last review:
Please also squash your fixup commits into the main commit that you're proposing to be merged.
If you need help with that, please see our Git guide to tidying commits, and if you still need help please join us in #git help in the Zulip development community.
khanak0509
force-pushed
the
fix-empty-topic-list-2003
branch
from
2e6356b to
4f5080d
Compare
|
I did : why this is happening ? could you please help me |
|
I think now there is only 1 commit([topic_list: Add empty state placeholder for channels with no topics]) . is it ok ? |
3 participants
Fixes #2003
When a channel has no topics, the topic list page now shows a helpful
placeholder message [("There are no topics here yet") , ("Be the first to start a conversation in this channel!")]
instead of an empty screen.
This improves the user experience by providing visual feedback .
issue #2003 .