Skip to content

msglist: Fix channel header tap behaviour in multi-channel narrows #1370

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.

Sign up for GitHub

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

Jump to bottom
Merged
gnprice merged 1 commit into from
Dec 15, 2025
Merged

msglist: Fix channel header tap behaviour in multi-channel narrows #1370

gnprice merged 1 commit into from
Dec 15, 2025
+104 −0

Conversation

@E-m-i-n-e-n-c-e
Copy link
Contributor

@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e commented Feb 21, 2025 •
edited
Loading

Description

This PR fixes the channel header navigation in multi-channel narrows

Changes

  • Modified the GestureDetector in StreamMessageRecipientHeader to use HitTestBehavior.opaque
  • Added a test that verifies tapping 1 px below the top of the recipient header, and 1 px above the bottom of it in the channel name area navigates to the channel feed
  • Also added a test that verifies tapping 1 px below the top of the recipient header, and 1 px above the bottom of it in the topic name area navigates to the topic feed

Related Issues:

Closes #1179

I have verified that the test fails before the changes and the test passes after the changes

@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e force-pushed the 1368 branch 3 times, most recently from 431f1cd to 81c835a Compare February 21, 2025 16:04
@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e mentioned this pull request Feb 22, 2025
Closed
@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e changed the title combined feed: Fix channel header tap behaviour in combined feed combined feed: Fix channel header tap behaviour in multi-channel narrows Feb 24, 2025
@E-m-i-n-e-n-c-e
Copy link
Contributor Author

E-m-i-n-e-n-c-e commented Feb 24, 2025

Chat thread

@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e force-pushed the 1368 branch 2 times, most recently from 4ab7f1b to 5f22c99 Compare February 25, 2025 08:22
@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e changed the title combined feed: Fix channel header tap behaviour in multi-channel narrows msglist: Fix channel header tap behaviour in multi-channel narrows Feb 25, 2025
@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e force-pushed the 1368 branch 2 times, most recently from d60ff5d to 57e46a7 Compare February 25, 2025 11:55
@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Feb 25, 2025
PIG208
PIG208 reviewed Feb 26, 2025
View reviewed changes
Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Left some comments.

@E-m-i-n-e-n-c-e
Copy link
Contributor Author

E-m-i-n-e-n-c-e commented Feb 27, 2025 •
edited
Loading

Pushed the revision. PTAL, Thanks.

@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e force-pushed the 1368 branch 2 times, most recently from 58b9eb0 to 72ffffd Compare February 28, 2025 12:55
PIG208
PIG208 reviewed Feb 28, 2025
View reviewed changes
Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update! Did another round with mostly small comments.

PIG208
PIG208 reviewed Feb 28, 2025
View reviewed changes
@E-m-i-n-e-n-c-e
Copy link
Contributor Author

E-m-i-n-e-n-c-e commented Mar 1, 2025 •
edited
Loading

@PIG208 I have pushed a revision addressing all the comments except for one comment I couldn't understand which I left unresolved. PTAL. Thanks!

@E-m-i-n-e-n-c-e
Copy link
Contributor Author

E-m-i-n-e-n-c-e commented Mar 3, 2025

Pushed another update. This one addresses all the review comments

@PIG208 PIG208 self-requested a review March 6, 2025 18:56
@PIG208
Copy link
Member

PIG208 commented Mar 6, 2025

Thanks for the update! This LGTM; marking for Greg's review. Because this resolves a post-launch issue, expect that it might take longer for us to eventually merge this while we prioritize launch tasks.

@PIG208 PIG208 removed the maintainer review PR ready for review by Zulip maintainers label Mar 6, 2025
@PIG208 PIG208 added the integration review Added by maintainers when PR may be ready for integration label Mar 6, 2025
@PIG208 PIG208 assigned gnprice and unassigned PIG208 Mar 6, 2025
@PIG208 PIG208 requested review from gnprice and removed request for PIG208 March 6, 2025 21:47
@gnprice gnprice added this to the M6: Post-launch milestone Mar 8, 2025
@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e force-pushed the 1368 branch 2 times, most recently from b401b9e to f06675c Compare June 13, 2025 10:19
@gnprice gnprice modified the milestones: MXA: Later, M7: 2025Q4 Dec 15, 2025
@E-m-i-n-e-n-c-e @gnprice
msglist: Fix channel header tap behavior in multi-channel narrows
52a7051 
Set gesture detecter behavior of streamWidget to HitTestBehavior.opaque
to handle taps in empty space around the header.

Fixes zulip#1179.
@gnprice
Copy link
Member

gnprice commented Dec 15, 2025

Thanks @E-m-i-n-e-n-c-e for taking care of this, and @PIG208 for all the previous reviews!

Just picking this up now, and it looks good; merging. I appreciate the careful tests which should make sure that any future changes to this layout don't break this without us noticing.

Fixed one nit in the commit message:

-    msglist: Fix channel header tap behaviour in multi-channel narrows
+    msglist: Fix channel header tap behavior in multi-channel narrows

The American spelling "behavior" is what's in the API we're using, and it's generally the standard in Flutter upstream and in Zulip.

@gnprice gnprice merged commit 52a7051 into zulip:main Dec 15, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gnprice gnprice Awaiting requested review from gnprice

1 more reviewer

@PIG208 PIG208 PIG208 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@gnprice gnprice

Labels

integration review Added by maintainers when PR may be ready for integration

Projects

None yet

Milestone

M7: 2025Q4

Development

Successfully merging this pull request may close these issues.

Tap target for channel in recipient header is shorter than header

3 participants

@E-m-i-n-e-n-c-e @PIG208 @gnprice