Skip to content

Conversation

@PVince81
Copy link
Member

@PVince81 PVince81 commented Jun 23, 2021

We should scroll as soon as we have the old messages / context loaded.
For newer messages, don't scroll up again to the read marker in case
there is one.

It happened to me on c.nc.com that even though I scrolled down after seeing the read marker, the chat scrolled up again to focus on the read marker. I suspect that it's related to the call I've removed in this PR.

@nickvergessen this is what we discussed yesterday and weren't sure if we wanted to keep. Let's remove it for now.

I'll retest:

  • initial scroll to read marker still works
  • initial scroll to search result still works
  • initial scroll to bottom works when no unread messages

@PVince81 PVince81 added this to the 💙 Next Major (22) milestone Jun 23, 2021
@PVince81 PVince81 self-assigned this Jun 23, 2021
@PVince81 PVince81 marked this pull request as ready for review June 23, 2021 16:06
@PVince81 PVince81 requested a review from nickvergessen June 23, 2021 16:24
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

This is in case of a room change. It breaks for me when switching between conversations.
But it is correct when I focus another tab and go back
Peek 2021-06-24 08-22

@PVince81
Copy link
Member Author

Hmmm, so maybe we need to add this in yet another code path. My guest is that it seemed to work before just by chance since we always call lookForNewMessages after switching conversations. I guess we probably don't always call getOldMessages when switching and keep them cached ? I'll recheck this

@nickvergessen
Copy link
Member

I guess we probably don't always call getOldMessages when switching and keep them cached ? I'll recheck this

yeah why should we, we still have them all loaded

We should scroll as soon as we have the old messages / context loaded.
For newer messages, don't scroll up again to the read marker in case
there is one.

Renamed getMessages for more clarity.

Signed-off-by: Vincent Petry <[email protected]>
@PVince81 PVince81 force-pushed the bugfix/noid/only-focus-once branch from 5cc8873 to 0767c88 Compare June 24, 2021 09:10
@PVince81
Copy link
Member Author

I guess we probably don't always call getOldMessages when switching and keep them cached ? I'll recheck this

yeah why should we, we still have them all loaded

alright, that was my misunderstanding but now from the code I see it's obvious.

I've fixed the focus logic to always be:

  1. get old messages (if necessary)
  2. set focus
  3. poll new messages

I've adjusted the function namings to avoid future misunderstandings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants