Skip to content
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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from

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
Open
@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

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.

@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
msglist: Fix channel header tap behaviour in multi-channel narrows
572b1c2 
Set gesture detecter behavior of streamWidget to HitTestBehavior.opaque
to handle taps in empty space around the header.

Fixes zulip#1179.
@E-m-i-n-e-n-c-e
Copy link
Contributor Author

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 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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PIG208 PIG208 PIG208 left review comments

@gnprice gnprice Awaiting requested review from gnprice

Assignees

@gnprice gnprice

Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Milestone
M6: Post-launch
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