Show channel/topic at top of channel/topic action sheet #1877

chrisbobbe · 2025-09-27T01:31:24Z

Outdated screenshots; working on new ones…

Channel	Topic

Light (general chat)	Dark (general chat)

Globe, short letter	Globe, tall letter

Hash, short letter	Hash, tall letter

Lock, short letter	Lock, tall letter

Long topic (light)	Long topic (dark)

Unknown channel	Unknown channel, with topic

rajveermalviya

Thanks @chrisbobbe! LGTM, and tests great. Moving over to Greg's review.

gnprice

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).

gnprice · 2025-10-02T01:12:46Z

lib/widgets/action_sheet.dart

  ];

-  _showActionSheet(pageContext, buttonSections: buttonSections);
+  Widget header;


nit: add final

gnprice · 2025-10-02T01:18:53Z

lib/widgets/action_sheet.dart

+    header = BottomSheetHeader(
+      buildTitle: (baseStyle) => Text(
+        style: baseStyle.copyWith(fontStyle: FontStyle.italic),
+        store.realmEmptyTopicDisplayName));


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.

Sure: #1896

alya · 2025-10-02T23:17:16Z

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.

alya · 2025-10-02T23:18:18Z

Looks good to me otherwise!

chrisbobbe · 2025-10-07T20:01:55Z

Thanks for the reviews! Revision pushed @alya, this time including the channel name in the topic action sheet too. I've also subtly adjusted the size and alignment of the channel icon; PTAL at that.

Greg commented that the line height looks too small when the topic wraps onto a second line. I kind of agree, but it looks just right to me in the common case where it doesn't wrap; in that case I think it might look awkward to add extra space above or below the line.

gnprice

Thanks! The new InlineIconGeometryData and iconWidgetData are interesting.

In the screenshots, the hash icon looks a bit low to me.

gnprice · 2025-10-08T00:13:29Z

lib/widgets/text.dart

+    ZulipIcons.hash_sign: InlineIconGeometryData._(
+      sizeFactor: 0.8,
+      alphabeticBaselineFactor: 1 / 8,
+      paddingFactor: 1 / 5),


Interesting!

Are there other places we should be using these values? The recipient headers, and message-list app bars, come to mind.

I agree! I think we can use it basically anywhere we have a channel name with its icon.

channelTopicLabelSpan in its current form isn't ready to be used in recipient headers, but maybe with adjustments. The channel name and topic need to have separate gesture handling, the chevron-right icon needs to be a different color than the text, and ideally we'd do something smart when there's a really long channel name but we still want to show the topic (or at least part of it, if it's long too).

gnprice · 2025-10-08T00:14:30Z

lib/widgets/text.dart

+  /// How much padding should separate the icon from surrounding text,
+  /// as a fraction of the text's font size.
+  final double paddingFactor;


Horizontal padding, right? Seems good to make explicit.

gnprice · 2025-10-08T00:16:26Z

lib/widgets/text.dart

+    required this.alphabeticBaselineFactor,
+    required this.paddingFactor,


It looks like the baseline factor ends up getting interpreted compounded with the size factor, but the padding factor doesn't.

Is there a motivation for that difference? Seems like it'd be easy to get confused about. It might be cleanest to specify both of them the same way (either both relative to the surrounding font size, or both relative to the icon size).

gnprice · 2025-10-08T00:17:58Z

lib/widgets/text.dart

+      paddingFactor: 0),
+ };
+
+  static InlineIconGeometryData defaultGeometry = InlineIconGeometryData._(


nit: make private?

gnprice · 2025-10-08T00:19:38Z

lib/widgets/text.dart

+    if (channelIcon != null) ...[
+      iconWidgetSpan(


nit: can skip the ...[, since there's exactly one item inside

alya · 2025-10-08T05:14:42Z

Side point: #design > unknown channel symbol

alya · 2025-10-08T05:47:24Z

Greg commented that the line height looks too small when the topic wraps onto a second line. I kind of agree, but it looks just right to me in the common case where it doesn't wrap; in that case I think it might look awkward to add extra space above or below the line.

Yeah, I agree that it looks to tight for 2-line topics, and fine without line-wrapping. Can we just increase the spacing when it wraps?

It might be OK to add a bit of space below for 1-line topics if we have to.

chrisbobbe · 2025-10-09T00:01:43Z

Thanks for the reviews! ~~Revision pushed~~ (edit: er, now it's pushed) and here are the new screenshots.

Can we just increase the spacing when it wraps?

Yes, I managed to do this! :)

Screenshots

Light	Dark

chrisbobbe · 2025-10-09T00:04:01Z

Ah and screenshots with a lock icon, almost forgot:

Light	Dark

gnprice · 2025-10-09T01:22:00Z

Thanks for the revision! The code all looks good to me; I'll hold off a bit on merging to give @alya an opportunity to look at the screenshots.

Perhaps file a follow-up issue for taking advantage of the new icon-geometry data, per #1877 (comment) ?

chrisbobbe · 2025-10-09T23:15:36Z

Perhaps file a follow-up issue for taking advantage of the new icon-geometry data, per #1877 (comment) ?

Sure: #1906

alya · 2025-10-11T00:28:35Z

Looks great to me, thanks!

…annel

This excludes the channel description for now; that's zulip#1896. Fixes-partly: zulip#1533

It turns out we can do this without wasting space with extra margin above or below the title, using TextHeightBehavior.

Fixes zulip#1533.

chrisbobbe requested a review from rajveermalviya September 27, 2025 01:31

chrisbobbe assigned rajveermalviya Sep 27, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Sep 27, 2025

chrisbobbe force-pushed the pr-channel-topic-action-sheet-headers branch from 7f6a80d to 96b3eba Compare September 27, 2025 01:32

rajveermalviya approved these changes Sep 30, 2025

View reviewed changes

rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Sep 30, 2025

rajveermalviya assigned gnprice and unassigned rajveermalviya Sep 30, 2025

rajveermalviya requested a review from gnprice September 30, 2025 14:19

gnprice reviewed Oct 2, 2025

View reviewed changes

chrisbobbe force-pushed the pr-channel-topic-action-sheet-headers branch from 96b3eba to 09bfe18 Compare October 7, 2025 19:39

gnprice reviewed Oct 8, 2025

View reviewed changes

chrisbobbe force-pushed the pr-channel-topic-action-sheet-headers branch from 09bfe18 to c18d1c4 Compare October 9, 2025 00:02

This was referenced Oct 9, 2025

Be consistent in aligning channel icon with channel name #1906

Open

RTL bug in channel-and-topic label span #1908

Open

chrisbobbe added 4 commits October 10, 2025 17:47

action_sheet [nfc]: Move an 8px top padding into BottomSheetHeader

5da93b2

action_sheet [nfc]: Dartdoc _showActionSheet

3f8f832

action_sheet: Add ActionSheetHeader.outerVerticalPadding

7d985ea

action_sheet [nfc]: Give action-sheet scrolling code more breathing room

9651e41

chrisbobbe added 5 commits October 10, 2025 17:47

action_sheet: Add headerScrollable param to _showActionSheet

8d43acf

action_sheet test: Support channel action sheet tests with unknown ch…

6795e96

…annel

action_sheet: Show channel name in channel action sheet

1a43fe2

This excludes the channel description for now; that's zulip#1896. Fixes-partly: zulip#1533

action_sheet: Increase line spacing in bottom-sheet title

536aa99

It turns out we can do this without wasting space with extra margin above or below the title, using TextHeightBehavior.

action_sheet: Show topic in topic action sheet

3d3b1c1

Fixes zulip#1533.

gnprice force-pushed the pr-channel-topic-action-sheet-headers branch from c18d1c4 to 3d3b1c1 Compare October 11, 2025 00:47

gnprice merged commit 3d3b1c1 into zulip:main Oct 11, 2025
1 check passed

chrisbobbe deleted the pr-channel-topic-action-sheet-headers branch October 11, 2025 01:28

		required this.alphabeticBaselineFactor,
		required this.paddingFactor,

Show channel/topic at top of channel/topic action sheet #1877

Show channel/topic at top of channel/topic action sheet #1877

Conversation

chrisbobbe commented Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alya commented Oct 2, 2025

Uh oh!

alya commented Oct 2, 2025

Uh oh!

chrisbobbe commented Oct 7, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alya commented Oct 8, 2025

Uh oh!

alya commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Oct 9, 2025

Uh oh!

gnprice commented Oct 9, 2025

Uh oh!

chrisbobbe commented Oct 9, 2025

Uh oh!

alya commented Oct 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chrisbobbe commented Sep 27, 2025 •

edited

Loading

alya commented Oct 8, 2025 •

edited

Loading

chrisbobbe commented Oct 9, 2025 •

edited

Loading