Skip to content

msglist: Let recipient header overlap date, just like a message #480

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

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Jan 8, 2024

Fixes: #479

Also update some tests that probably should have been updated in #469.


Screenshots

Before:
image

After:
image

@gnprice gnprice added the a-msglist The message-list screen, except what's label:a-content label Jan 8, 2024
@gnprice gnprice requested a review from chrisbobbe January 8, 2024 21:47
@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Jan 8, 2024

Thanks! I noticed the behavior of #479 too, when reviewing #469, but it didn't stand out to me as a problem. Now I see what you're saying about how the #479 behavior allows some information (like a message's topic) to get nudged off the screen.

This alternative approach seems mostly fine, except I think it now allows the appearance of incorrect information: if I'm looking at the first message on a new day, I can scroll up slowly until it looks like that same message is being labeled with a previous day (like "Yesterday"):

RPReplay_Final1704753987.mov

I don't yet see a nice way to fix that and #479 at the same time, so if you think this is OK, please merge at will.

@gnprice
Copy link
Member Author

gnprice commented Jan 8, 2024

Thanks for the review!

This alternative approach seems mostly fine, except I think it now allows the appearance of incorrect information: if I'm looking at the first message on a new day, I can scroll up slowly until it looks like that same message is being labeled with a previous day (like "Yesterday"):

Yeah. I'm pretty sure Zulip web had an equivalent bug for years; it's not great, but I think it's a lot less conspicuous than #479. The latter not only pushes some information off the screen, but looks like a significant visual glitch because (a) if you're moving quickly, there's a big flash of white where the sticky header should be, and (b) when the sticky header finishes getting pushed off the screen, "it" (really the new one with the new date) suddenly reappears all at once.

I should file an issue for that new problem, though, so we can track it. (→ #481.) I think fixing it will probably involve adding a further wrinkle to the sticky_header library.

This check in checkInvariants wasn't updated when we added
date separators recently; and the reason nothing broke is because
we didn't have a test to exercise that, oops.

Update the invariant, and extend the relevant test to exercise it.
Now the new test would fail if run with the old checkInvariants.
This test of how we maintain recipient headers through various
changes had some lingering references to `canShareRecipientHeader`,
which was recently split up and removed.  Refresh the comments,
and also add a TODO comment about writing a similar test (or
perhaps extending this test) for date separators.
@gnprice gnprice merged commit 982bbee into zulip:main Jan 8, 2024
@gnprice gnprice deleted the pr-dates branch January 8, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-msglist The message-list screen, except what's label:a-content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date separators push sticky header off screen
2 participants