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: In single-conversation view, make recipient headers not tappable. #1193

Merged
merged 2 commits into from
Jan 29, 2025
Merged

msglist: In single-conversation view, make recipient headers not tappable. #1193

merged 2 commits into from
Jan 29, 2025

Conversation

lakshya1goel
Copy link
Contributor

Pull Request Description

Summary
This PR updates the behavior of recipient headers in the single-conversation view to make them non-tappable.

Related Issue: #1171

Changes:

Adjusted GestureDetector logic:
Updated the logic to conditionally enable navigation only when relevant.

Simplified ColoredBox structure:
Streamlined the layout for non-tappable headers to ensure consistent behavior and appearance.

Video Demonstration:

WhatsApp.Video.2024-12-20.at.10.39.41.PM.mp4

chrisbobbe
chrisbobbe requested changes Dec 20, 2024
View reviewed changes
Copy link
Collaborator

@chrisbobbe chrisbobbe 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! It needs tests, and the commit message should say "Fixes" with the issue number. Also see two substantive comments below.

@lakshya1goel lakshya1goel force-pushed the issue1171 branch 2 times, most recently from 59f7f42 to eb6a565 Compare December 21, 2024 10:13
@lakshya1goel
Copy link
Contributor Author

@chrisbobbe Please have a look on the changes now. Added tests as well. But getting the CI/check fails, showing

Failed to download https://storage.googleapis.com/flutter_infra_release/flutter/7df127b1ee7d52c4cb0d42eedd00190e2c4ade3d/flutter_patched_sdk_product.zip. Ensure you have network connectivity and then try again.
Exception: 404
Error: Process completed with exit code 1.

Please let me know how can I fix them, as I have the good internet connectivity still getting this.

@lakshya1goel
Copy link
Contributor Author

@chrisbobbe , the checks have also been fixed now. Please review it once.
Thanks!

@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Dec 23, 2024
chrisbobbe
chrisbobbe reviewed Dec 23, 2024
View reviewed changes
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! Small comments below.

Also a commit-message nit:

msglist: In single-conversation view, make recipient headers not tappable.

Updated onTap for recipient headers in single-conversation view.
Adjusted GestureDetector logic to conditionally enable navigation.
Simplified ColoredBox structure for non-tappable recipient headers.
Improves user experience by removing unnecessary tap interactions.

Fixes: #1171

The paragraph isn't needed; it doesn't add anything that isn't already easy to see from the code change, or implied by the Fixes: #1171 line.

@lakshya1goel lakshya1goel force-pushed the issue1171 branch 2 times, most recently from a671aee to 83b47de Compare December 24, 2024 04:12
@lakshya1goel
Copy link
Contributor Author

Hello, @chrisbobbe I have done the requested changes. Please have a look on them.
Thanks!

chrisbobbe
chrisbobbe reviewed Dec 26, 2024
View reviewed changes
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! Small comments below.

@lakshya1goel
Copy link
Contributor Author

Hello @chrisbobbe, done with the changes mentioned in above comments.
Please take a moment to review them.
Thanks!

chrisbobbe
chrisbobbe reviewed Dec 30, 2024
View reviewed changes
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! Small comments below.

@lakshya1goel
Copy link
Contributor Author

Hi, I have made the required changes. Please have a look on them.
Thanks!

chrisbobbe
chrisbobbe reviewed Jan 2, 2025
View reviewed changes
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! Comments below from a closer review, all small.

@lakshya1goel
Copy link
Contributor Author

Thanks for the detailed review. Done with all the changes please have a look on it.
Thanks!

@chrisbobbe chrisbobbe self-assigned this Jan 3, 2025
chrisbobbe
chrisbobbe reviewed Jan 4, 2025
View reviewed changes
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! Small nits below, otherwise LGTM.

@lakshya1goel
Copy link
Contributor Author

Thanks for the review. I have made the changes. Please have a look.
Thanks!

@lakshya1goel
Copy link
Contributor Author

Hi @chrisbobbe,
I just wanted to kindly follow up on this PR. These are the final changes, and I requested a review about a week ago. If you could take a moment to review it, I would greatly appreciate your feedback.
Thank you!

@lakshya1goel
Copy link
Contributor Author

Hi @chrisbobbe, mentioning you here in case you have missed this PR. Please have a look whenever you get some time.
Thanks!

@chrisbobbe
Copy link
Collaborator

Hi, sorry I lost track of this for a bit! Marking for Greg's review.

@chrisbobbe chrisbobbe assigned gnprice and unassigned chrisbobbe Jan 22, 2025
@chrisbobbe chrisbobbe 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 Jan 22, 2025
@chrisbobbe chrisbobbe requested a review from gnprice January 22, 2025 22:01
gnprice
gnprice reviewed Jan 23, 2025
View reviewed changes
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @lakshya1goel for making this change, and @chrisbobbe for all the previous reviews!

Generally this approach looks good. Several comments below.

@gnprice gnprice mentioned this pull request Jan 23, 2025
Merged
@lakshya1goel lakshya1goel requested a review from gnprice January 24, 2025 08:56
@lakshya1goel
Copy link
Contributor Author

Thanks for the detailed review @gnprice! I have made the required changes. Please letme know if anything else is required.

gnprice
gnprice reviewed Jan 27, 2025
View reviewed changes
Copy link
Member

@gnprice gnprice 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 revision! The implementation changes now all look good modulo nits. Comments below, mainly on the tests.

@lakshya1goel lakshya1goel requested a review from gnprice January 28, 2025 08:08
@lakshya1goel
Copy link
Contributor Author

lakshya1goel commented Jan 28, 2025 •
edited
Loading

Thanks for the detailed review again @gnprice and apologies for missing out some comments of the previous review.
I have pushed the revision, please have a look on it.

gnprice
gnprice reviewed Jan 29, 2025
View reviewed changes
Copy link
Member

@gnprice gnprice 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 revision! All looks good now except a couple of nits. Then this will be ready to merge.

@lakshya1goel lakshya1goel requested a review from gnprice January 29, 2025 05:17
@lakshya1goel
Copy link
Contributor Author

Thanks @gnprice, pushed the changes, PTAL.

@lakshya1goel @gnprice
msglist [nfc]: Move _containsDifferentChannels to StreamMessageRecipi…
f4b0b80 
…entHeader

Move _containsDifferentChannels method to StreamMessageRecipientHeader
where it's used, and pass narrow directly instead of a pre-computed
showStream boolean.
@lakshya1goel @gnprice
msglist: In single-conversation view, make recipient headers not tapp…
abec662 
…able.

Fixes: zulip#1171
@gnprice
Copy link
Member

gnprice commented Jan 29, 2025

Thanks! Looks good; merging.

@gnprice gnprice merged commit abec662 into zulip:main Jan 29, 2025
@gnprice gnprice mentioned this pull request Feb 26, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisbobbe chrisbobbe Awaiting requested review from chrisbobbe

@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
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lakshya1goel @chrisbobbe @gnprice