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

restore scroll position after loading previous messages #496

Merged
merged 1 commit into from
Jul 28, 2016

Conversation

davibe
Copy link
Contributor

@davibe davibe commented Jul 12, 2016

No description provided.

@astorije astorije self-assigned this Jul 12, 2016
@astorije astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jul 12, 2016
@williamboman
Copy link
Member

williamboman commented Jul 13, 2016

Works fine for me functionality-wise on desktop and mobile. Since the Show More button is hidden when one reaches the end of the message stack, the offset calculation is a bit off when clicking Show More the last time which causes minor flicker.

Are there any content that is/could be rendered asynchronously, effectively skewing the height offset? I can't think of any except thumbnails and possibly links, but those aren't expanded on old messages afaik.

@astorije
Copy link
Member

Since the Show More button is hidden when one reaches the end of the message stack, the offset calculation is a bit off when clicking Show More the last time which causes minor flicker.

Good catch. @davibe, is it difficult to take this into account?

Are there any content that is/could be rendered asynchronously, effectively skewing the height offset?

Not at the moment, I believe.

I can't think of any except thumbnails and possibly links, but those aren't expanded on old messages afaik.

This seems like a bug, actually, and should be fixed.
I think we should think of the consequences of fixing this though. All the solutions I can think of right now aren't too great: caching the N+1 call to "Show more" and displaying N, previously loaded and thumbnails generated; pausing the display until everything is loaded (eww!).
Any better ideas?

@davibe
Copy link
Contributor Author

davibe commented Jul 14, 2016

For the flickering caused by button.. we can just remove the button before doing calculations.

Dynamically (async) loaded contents should just have a fixed height (css) and, if its needed, they could expand on click one by one.

@maxpoulin64
Copy link
Member

Looks fine to me. Please squash those commits and then I'll approve this. If we can fix the small shifting when the last messages are loaded that would be cool as well but this works fine for me as it is.

@astorije
Copy link
Member

Agreed with @maxpoulin64, can you squash these commits please, @davibe?
We can improve that bump on the oldest messages, no big deal. It's still much better than it is right now :)

@astorije
Copy link
Member

@davibe, ping? :)

@davibe
Copy link
Contributor Author

davibe commented Jul 27, 2016

I reduced to 1 commit

@maxpoulin64
Copy link
Member

👍

@astorije
Copy link
Member

👍 and merging.

@astorije astorije merged commit a7fe19d into thelounge:master Jul 28, 2016
@astorije astorije modified the milestone: 2.0.0 Aug 6, 2016
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
restore scroll position after loading previous messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants