Hide topic input on a general chat only channel #1984
Conversation
chungwwei
force-pushed
the
hide-topic-input
branch
2 times, most recently
from
d6f17cd to
a938e0c
Compare
chungwwei
force-pushed
the
hide-topic-input
branch
from
a938e0c to
4498902
Compare
|
two things I noticed while working on this pr.
|
rajveermalviya
added
the
maintainer review
rajveermalviya
left a comment
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks for working on this @chungwwei! Initial comments below.
two things I noticed while working on this pr.
when in channel narrow view, the top-right Topics icon can probably be hidden, since there is only one topic.
web treat the topic Input as read-only with text General Chat, so we should match that instead of hiding the topic input?
These are great questions for the #mobile-design channel, so please open a discussion and ask them there.
| streamId: narrow.streamId, | ||
| controller: controller, | ||
| ); | ||
| } |
There was a problem hiding this comment.
I did a quick manual test and noticed that ComposeBoxController.requestFocusIfUnfocused doesn't work correctly with this implementation. So when selecting the general-chat-only channel from Share-to-Zulip page, the content box doesn't get focused automatically and presumably the non-existent topic box is being focused instead.
There was a problem hiding this comment.
I am not sure what the correct implementation here would be. @gnprice @chrisbobbe Should this type of channel use the FixedDestinationComposeBoxController/_FixedDestinationComposeBoxBody directly?
There was a problem hiding this comment.
Good question. Yeah, I think the fixed-destination compose box is a good solution for preventing the sort of problem you spotted.
In principle that leaves a case which is a bit tricky, which is when the channel changes from single-topic to normal or vice versa while the compose box is open. I think it's fine if the compose box just doesn't react to that, and carries on as if the channel still had the old setting. That's going to be a rare case, and if the user eventually hits send at the end of it then they should get a coherent error message from the server, and have the opportunity to copy their attempted message text so they can try again.
| group('compose box inputs visibility by topic policy', () { | ||
| void testComposeBoxWidgetVisibility({required TopicsPolicy topicsPolicy, required bool shouldShowTopicInput}) { | ||
| final description = shouldShowTopicInput ? | ||
| 'show topic input when topic policy is $topicsPolicy': | ||
| 'hide topic input when topic policy is $topicsPolicy'; | ||
|
|
||
| testWidgets(description, (tester) async { | ||
| final channel = eg.stream(topicsPolicy: topicsPolicy); | ||
|
|
||
| await prepareComposeBox(tester, narrow: ChannelNarrow(channel.streamId), streams: [channel]); | ||
|
|
||
| check(contentInputFinder).findsOne(); | ||
| shouldShowTopicInput ? | ||
| check(topicInputFinder).findsOne(): | ||
| check(topicInputFinder).findsNothing(); | ||
| }); | ||
| } | ||
|
|
||
| testComposeBoxWidgetVisibility(topicsPolicy: TopicsPolicy.inherit, shouldShowTopicInput: true); | ||
| testComposeBoxWidgetVisibility(topicsPolicy: TopicsPolicy.allowEmptyTopic, shouldShowTopicInput: true); | ||
| testComposeBoxWidgetVisibility(topicsPolicy: TopicsPolicy.disableEmptyTopic, shouldShowTopicInput: true); | ||
| testComposeBoxWidgetVisibility(topicsPolicy: TopicsPolicy.emptyTopicOnly, shouldShowTopicInput: false); | ||
| }); |
There was a problem hiding this comment.
nit: line too long, please limit lines to 80 characters.
|
Thanks for reviewing @rajveermalviya! Apologies, I underestimated the time commitment and haven't been able to keep up due to other commitments. I maybe able to revisit this after the new year if the issue isn't resolved by someone else. |
3 participants
A follow up to #1956. Marking it as draft for now, will need to rebase on top of #1956.
Screenshot
Fixes: #1843