all_channels: Replace three dot menu with long press gesture and add on tap to feed #1915

chungwwei · 2025-10-11T09:10:19Z

Fixes #1914

Before and After still

Page View

Before	After

After Max Sys Font

After Max Sys Font

Long Press View

Long Press on Public Channel	~~Long Press on Private Channel~~ Not part of this PR anymore

Gesture Press Views

Gesture press in default sys font

screen-20251028-1754402.mp4

Unsub from private channel 200 milliseconds fade

screen-20251028-1749162.mp4

gnprice

Thanks @chungwwei for writing this PR! Generally this looks good. Various small comments below.

lib/widgets/all_channels.dart

test/widgets/all_channels_test.dart

gnprice · 2025-10-15T23:36:41Z

The screencast video is helpful, thanks.

Please also include a still screenshot. See discussion in our instructions:
https://zulip.readthedocs.io/en/latest/contributing/reviewable-prs.html#demonstrating-visual-changes

The interactions in the screencast are somewhat hard to follow, because the viewer can't see where on the screen you were tapping. When you take a screen recording, there's an option to have it show touches on the screen (here's Android docs); you should basically always enable that option when making a screencast to demonstrate some UI behavior.

gnprice · 2025-10-17T00:34:41Z

Thanks for the revision. One reply in a subthread above: #1915 (comment)

Please also add a screenshot, and a screen recording with touches visible, so that we can review the visual changes. See details in my comment above: #1915 (comment)

gnprice · 2025-10-17T00:37:00Z

Oh and please see our Git style guide:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#each-commit-must-be-coherent

In this PR, the two commits should be squashed together, rather than have one commit for the first revision and then another commit that adjusts those changes.

gnprice · 2025-10-18T02:46:03Z

Thanks for the revision.

Please see our Git style guide (linked above and in our README) for how to write clear commit messages. See also the reply above at #1915 (comment) .

In the screenshots, the toggles are much too close to the right edge of the screen. (Notice that they're visually much closer than the content at the left is to the left edge, and much closer than the three-dots icon is in main.) They should get appropriate padding.

chungwwei · 2025-10-18T14:09:39Z

Thanks Greg for the review! Hopefully the commits are following the style guide now.

I believe that GestureDetector was probably not the best fit to use in the PR. Other parts of the app like the Channel Page uses InkWell, which has the rippling effect when long press.

gnprice

Thanks for the revision, and the updated screenshots. Some comments below.

The commit messages are mostly good now, but the body should get line-wrapped. See this section:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#formatting-guidelines

To demonstrate why, here's how the first commit message looks when I'm reviewing the changes (with git log --stat -p):

lib/widgets/all_channels.dart

gnprice · 2025-10-21T00:00:21Z

lib/widgets/all_channels.dart

+        child: SizedBox(
+          height: 38,


Hmm, I just noticed this part — a fixed height like this can't be right.

Try setting your system's font size setting to larger text, and you'll see why.

This is also too short. Touch targets should be at least 44px high so it's not too difficult to tap the right one. In your screenshots, these rows are getting shorter; they should stay the same height unless there's a clear reason we're choosing to change the height.

🤯Ahh, I didn’t notice the subtle details. The text would get truncated if system font size is large enough.

The motivation for the SizedBox was to keep the row height same for all row item even when the toggle isn’t shown and/or when text gets too large. UI jump can happen when a user unsub from a private channel.

/* Maintain layout whether or not the toggle is shown. SizedBox(height: 40, width: 52) to match the Size of _SubscribeToggle. */ hasContentAccess ? _SubscribeToggle(channel: channel) : const SizedBox(height: 40, width: 52),

I believe height is currently 40. Touch target should be at least 44 px high can be fixed by adding top and bottom padding.

The other thing I just noticed is that, the row height gets taller when the channel name is too long and/or font is large. I should restrict text to one line and ellipsis on overflow just like how the Channel Page does it.

Make the All Channels Page consistent with the Channel Page, as navigating to channel feed and opening the bottom sheet currently uses the three-dot menu icon, which differs from the gesture pattern used in the Channel Page. Fixes part of zulip#1914

Previously, long name could span multiple lines when the system font size was large, causing the row height to become insconsistent with other items. Limiting the name to a single line ensures consistent row height, similar to how it's done on the Channel Page. Fixes part of zulip#1914

gnprice

Thanks for the revision!

gnprice · 2025-10-27T05:02:35Z

lib/widgets/all_channels.dart

+          narrow: ChannelNarrow(channel.streamId))),
+      onLongPress: () => showChannelActionSheet(context, channelId: channel.streamId),
+      child: Padding(
+        padding: const EdgeInsets.fromLTRB(8, 2, 12, 2),


This should use fromSTEB, not fromLTRB, so that it works correctly for people using RTL languages. See the EdgeInsets doc. (Note also the "start" and "end" in the existing code, vs. "left" and "right".)

gnprice · 2025-10-27T05:07:43Z

lib/widgets/all_channels.dart

+                height: 20 / 17,
+              ).merge(weightVariableTextStyle(context, wght: 600)),
+              channel.name)),
+          hasContentAccess ? _SubscribeToggle(channel: channel) : const SizedBox(height: 40, width: 52)


Add SizedBox(height: 40, width: 52), the size of the toggle, as a placeholder for hidden toggle to prevent UI jump. Adjust Row padding

This seems pretty fragile.

Is the toggle always 40px high, or might it sometimes be taller or shorter? It's not obvious to me that it's definitely always the same height.

Even if it's currently always that height, will it always be that height in the future? That doesn't sound like something I'd expect it to guarantee.

~~System display can change the toggle size.~~ Can't tell if thats the case, debugging shows me its the same height throughout.

Instead of top and bottom padding, it might be better to use ConstrainedBox(constraints: const BoxConstraints(minHeight: 44), so there's no guessing needed to make row item at least 44px.

InkWell( child: ConstrainedBox(constraints: const BoxConstraints(minHeight: 44), child: ... ));

Idk if it's considered hacky, but making it transparent and non-interactive is a simple way to prevent ui jump. Is this an acceptable alternative? This way, the toggle's dimension don't need to be taken into account.

IgnorePointer(ignoring: !hasContentAccess, child: AnimateOpacity(opacity: !hasContentAccess ? 0 : 1, duration: Duration(milliseconds: 200), // so that it's not abruptly disappear child: _SubscribeToggle(channel: channel)))

Updated a screen recording that shows unsub from a private channel up top.

Make sure every row is at least 44px in height by specifying BoxConstraints(minHeight: 44) to meet touch target requirement.

noContentAccess If the toggle's height exceeds 44px, unsubsribing from a private channel shrinks the row to 44px, causing a ui jump. If the private channel name is too long, unsubscribing casues the previously ellipsis text to stretch to the right edge, casuing a ui jump.

gnprice

Thanks for all your work on this PR! I think this is very close now.

gnprice · 2025-10-31T00:24:12Z

lib/widgets/all_channels.dart

+            IgnorePointer(ignoring: !hasContentAccess,
+              child: AnimatedOpacity(opacity: !hasContentAccess ? 0 : 1,
+                duration: Duration(milliseconds: 200),
+                child: _SubscribeToggle(channel: channel)))


nit: always use trailing comma, unless ) is on the same line

Suggested change

child: _SubscribeToggle(channel: channel)))

child: _SubscribeToggle(channel: channel))),

(This will also slightly reduce the diff in the third / next-to-last commit.)

gnprice · 2025-10-31T00:34:05Z

lib/widgets/all_channels.dart

+                maxLines: 1,
+                overflow: TextOverflow.ellipsis,


nit:

all_channels: Restrict channel name to oneline

"one line" is two words; "oneline" isn't a word

separate nit:

Fixes part of #1914

If this is the commit that fixes the last remaining part of #1914, then it's the commit that causes #1914 to become fixed. So in that case the commit message should say that it fixes #1914.

gnprice · 2025-10-31T00:36:29Z

lib/widgets/all_channels.dart

+        MessageListPage.buildRoute(context: context,
+          narrow: ChannelNarrow(channel.streamId))),
+      onLongPress: () => showChannelActionSheet(context, channelId: channel.streamId),
+      child: ConstrainedBox(constraints: const BoxConstraints(minHeight: 44),


commit-sequence nit:

all_channels: Make row item at least 44px tall with ConstrainedBox Make sure every row is at least 44px in height by specifying BoxConstraints(minHeight: 44) to meet touch target requirement.

Did the removal of the three-dots button make these rows shorter than they had been?

If so, then this commit adding the 44px min-height should go before the commit that removes the three-dots button. That way we don't have an intermediate state where the UI is less good in some ways than it was before.

gnprice · 2025-10-31T00:47:39Z

lib/widgets/all_channels.dart

+            IgnorePointer(ignoring: !hasContentAccess,
+              child: AnimatedOpacity(opacity: !hasContentAccess ? 0 : 1,
+                duration: Duration(milliseconds: 200),
+                child: _SubscribeToggle(channel: channel)))


I'm not convinced we need this change, i.e. the last commit:
5a60145 all_channels: Make toggle transparent and non-interactive when noContentAccess

It's not a super common interaction in the first place (though it is an important one) to subscribe and unsubscribe to channels; and I think it's going to be a lot less common than that to unsubscribe when that means you lose access to the channel. So if that sometimes means that the row gets a bit shorter and the rows below jump up a bit, I think that's a pretty minor issue.

(It's also not clear to me whether that ever actually does happen, or if it's a theoretical issue.)

Conversely, having this here makes the code somewhat more complicated to understand. It also adds a nontrivial widget (the toggle) to the tree in cases where the user won't actually see it; and does so for each element of a potentially long list, so it has a potential performance cost.

gnprice added maintainer review PR ready for review by Zulip maintainers integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Oct 15, 2025

gnprice reviewed Oct 15, 2025

View reviewed changes

gnprice mentioned this pull request Oct 16, 2025

docs: Request showing touches/clicks in screencast videos zulip/zulip#36287

Merged

chungwwei requested a review from gnprice October 16, 2025 12:01

chungwwei force-pushed the all-channels-ui branch 3 times, most recently from 1a64215 to a3105d0 Compare October 17, 2025 16:58

chungwwei force-pushed the all-channels-ui branch 3 times, most recently from 698f215 to 4ae4515 Compare October 18, 2025 12:24

chungwwei force-pushed the all-channels-ui branch from 508fa04 to 173bf47 Compare October 18, 2025 18:12

gnprice reviewed Oct 21, 2025

View reviewed changes

chungwwei force-pushed the all-channels-ui branch 5 times, most recently from 1f3725d to e775639 Compare October 21, 2025 17:12

chungwwei requested a review from gnprice October 21, 2025 17:46

chungwwei force-pushed the all-channels-ui branch 5 times, most recently from 78b7ca3 to 3acba11 Compare October 26, 2025 02:07

chungwwei added 2 commits October 26, 2025 00:53

chungwwei force-pushed the all-channels-ui branch 2 times, most recently from 7bb92ae to dac2f4b Compare October 26, 2025 17:57

gnprice reviewed Oct 27, 2025

View reviewed changes

chungwwei force-pushed the all-channels-ui branch 2 times, most recently from 161dc25 to e7aaa87 Compare October 28, 2025 17:48

all_channels: Make row item at least 44px tall with ConstrainedBox

baf929f

Make sure every row is at least 44px in height by specifying BoxConstraints(minHeight: 44) to meet touch target requirement.

chungwwei force-pushed the all-channels-ui branch from e7aaa87 to ca4e951 Compare October 28, 2025 17:59

chungwwei force-pushed the all-channels-ui branch from ca4e951 to 5a60145 Compare October 28, 2025 18:43

chungwwei requested a review from gnprice October 28, 2025 22:04

gnprice reviewed Oct 31, 2025

View reviewed changes

	child: _SubscribeToggle(channel: channel)))
	child: _SubscribeToggle(channel: channel))),

all_channels: Replace three dot menu with long press gesture and add on tap to feed #1915

Are you sure you want to change the base?

all_channels: Replace three dot menu with long press gesture and add on tap to feed #1915

Conversation

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

Uh oh!

Page View

Gesture press in default sys font

Unsub from private channel 200 milliseconds fade

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gnprice commented Oct 15, 2025

Uh oh!

gnprice commented Oct 17, 2025

Uh oh!

gnprice commented Oct 17, 2025

Uh oh!

gnprice commented Oct 18, 2025

Uh oh!

chungwwei commented Oct 18, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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!

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chungwwei commented Oct 11, 2025 •

edited

Loading

chungwwei Oct 21, 2025 •

edited

Loading

chungwwei Oct 27, 2025 •

edited

Loading