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.

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

improvement: scroll to new messages smoothly #4125

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

Conversation

WofWca
Copy link
Collaborator

@WofWca WofWca commented Sep 12, 2024

TODO:

  • Test it and see if it gets stuck often enough
    for it to be critical. I have seen it happen a few times.
    Maybe for 1 out of 100 messages.
  • Think about putting behind an experimental setting?
    Ok, I have an idea. Let's merge it, and if it's not good yet, then put it behind the setting.
  • Add CHANGELOG entry.

@WofWca WofWca force-pushed the wofwca/smooth-scroll-to-new-message branch 3 times, most recently from e4d11e2 to 200682f Compare October 1, 2024 13:43
@WofWca WofWca marked this pull request as ready for review October 2, 2024 11:21
@WofWca WofWca changed the title WIP: improvement: scroll to new messages smoothly improvement: scroll to new messages smoothly Oct 5, 2024
@nicodh
Copy link
Contributor

nicodh commented Oct 17, 2024

Test it and see if it gets stuck often enough
for it to be critical. I have seen it happen a few times.

Can you describe how to test and provoke "critical" behaviour?

Maybe for 1 out of 100 messages.
So this means for testing: it needs that many repetitions to run into an failure? What kind of failure would that be?

@WofWca
Copy link
Collaborator Author

WofWca commented Oct 17, 2024

Can you describe how to test and provoke "critical" behaviour?

I mean the behavior we observed in #4186, #3763, #3753. By "critical" I mean whether the occasional "scroll up" outweighs the niceness of the smooth scroll, and so whether we want to merge this, without making it an experimental setting.

So this means for testing: it needs that many repetitions to run into an failure?

Yes, it results in a "scrolled up" chat once every ~100 smooth scrolls.

@nicodh
Copy link
Contributor

nicodh commented Oct 29, 2024

I mean the behavior we observed in #4186, #3763, #3753. By "critical" I mean whether the occasional "scroll up" outweighs the niceness of the smooth scroll, and so whether we want to merge this, without making it an experimental setting.

These bugs are all solved, so I still don't get what this PR fixes/improves

Can you add a video like "before" and "after"

TODO:
- [ ] Test it and see if it gets stuck often enough
  for it to be critical. I have seen it happen a few times.
  Maybe for 1 out of 100 messages.
- [ ] Think about putting behind an experimental setting?
@WofWca WofWca force-pushed the wofwca/smooth-scroll-to-new-message branch from 200682f to a8a8302 Compare October 29, 2024 08:36
@WofWca
Copy link
Collaborator Author

WofWca commented Oct 29, 2024

These bugs are all solved, so I still don't get what this PR fixes/improves

Those are solved, but this MR might introduce them back (I mean, not those bugs precisely, but their effect of "scrolling up").

I edited the original post by adding a video.

Copy link
Contributor

@nicodh nicodh left a comment

Choose a reason for hiding this comment

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

Works as expected! I consider it an improvement since it becomes more obvious that a new message arrived although it introduces a bit more complexity to the MessageList component whis has ~940 lines now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants