-
Notifications
You must be signed in to change notification settings - Fork 340
Show channel/topic at top of channel/topic action sheet #1877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Show channel/topic at top of channel/topic action sheet #1877
Conversation
Fixes zulip#1533. This excludes the channel description for now; that'll be more complicated to implement, involving zulip#488.
7f6a80d
to
96b3eba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chrisbobbe! LGTM, and tests great. Moving over to Greg's review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Small code comments below.
I feel like we might want to include the channel name/icon when showing the topic, too. Consider the case of the combined feed or another interleaved view.
Which reminds me: this should get UX review from Alya (if it hasn't already in a chat thread).
]; | ||
|
||
_showActionSheet(pageContext, buttonSections: buttonSections); | ||
Widget header; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add final
header = BottomSheetHeader( | ||
buildTitle: (baseStyle) => Text( | ||
style: baseStyle.copyWith(fontStyle: FontStyle.italic), | ||
store.realmEmptyTopicDisplayName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
action_sheet: Show topic in topic action sheet
Fixes #1533.
This excludes the channel description for now; that'll be more
complicated to implement, involving #488.
That last bit belongs in the commit message for the channel action sheet, right?
Let's also track that follow-up somewhere — either explicitly in #488, or as its own small issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure: #1896
Hmm, we don't show the channel name in topic menus in the web app, but you can seem more context around the menu there. Including the name of the channel makes sense to me for mobile. |
Looks good to me otherwise! |
Fixes #1533.