- 
                Notifications
    
You must be signed in to change notification settings  - Fork 349
 
all_channels: Replace three dot menu with long press gesture and add on tap to feed #1915
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
Conversation
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 @chungwwei for writing this PR! Generally this looks good. Various small comments below.
| 
           The screencast video is helpful, thanks. Please also include a still screenshot. See discussion in our instructions: 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.  | 
    
| 
           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)  | 
    
| 
           Oh and please see our Git style guide: 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.  | 
    
1a64215    to
    a3105d0      
    Compare
  
    | 
           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.  | 
    
698f215    to
    4ae4515      
    Compare
  
    | 
           Thanks Greg for the review! Hopefully the commits are following the style guide now. I believe that   | 
    
508fa04    to
    173bf47      
    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 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):
      
    1f3725d    to
    e775639      
    Compare
  
    78b7ca3    to
    3acba11      
    Compare
  
    7bb92ae    to
    dac2f4b      
    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 for the revision!
ca4e951    to
    5a60145      
    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 for all your work on this PR! I think this is very close now.
| MessageListPage.buildRoute(context: context, | ||
| narrow: ChannelNarrow(channel.streamId))), | ||
| onLongPress: () => showChannelActionSheet(context, channelId: channel.streamId), | ||
| child: ConstrainedBox(constraints: const BoxConstraints(minHeight: 44), | 
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.
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.
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.
Yeah, reordering the commits.
Private channel row with no content access is shorter (20px high) without the three-dots button.
        
          
                lib/widgets/all_channels.dart
              
                Outdated
          
        
      | IgnorePointer(ignoring: !hasContentAccess, | ||
| child: AnimatedOpacity(opacity: !hasContentAccess ? 0 : 1, | ||
| duration: Duration(milliseconds: 200), | ||
| child: _SubscribeToggle(channel: channel))) | 
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.
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.
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.
Went down the rabbit hole on this one, but learned a few things along the way, like the 44px touch targets and some git rebase tips.
Dropped the commit and updated the statics screenshots.
f8c4aad    to
    ef41ad2      
    Compare
  
    | 
           Thanks! This all looks good now — merging.  | 
    
Make sure every row is at least 44px in height by specifying BoxConstraints(minHeight: 44) to meet touch target requirement.
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 zulip#1914
ef41ad2    to
    8942c91      
    Compare
  
    
Fixes #1914
Before and After still
Page View
After Max Sys Font
Long Press View
Long Press on Private ChannelNot part of this PR anymoreGesture Press Views
Gesture press in default sys font
screen-20251028-1754402.mp4
Unsub from private channel 200 milliseconds fade. Not part of PRscreen-20251028-1749162.mp4