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

Fix/scroll to top on sort change #329

Merged
merged 12 commits into from
Jul 12, 2023

Conversation

mdecourcy
Copy link
Contributor

Closes #272

@mdecourcy
Copy link
Contributor Author

mdecourcy commented Jul 8, 2023

@aeharding I'm not home right now so can't look in to this myself, but before merging in it might be worth looking in to only scrolling to the top of the comments/bottom of the post when changing comment sort.

Unsure how apollo handled this, as I mainly used Infinity.

Could also add this in a later release.

@aeharding
Copy link
Owner

@aeharding I'm not home right now so can't look in to this myself, but before merging in it might be worth looking in to only scrolling to the top of the comments/bottom of the post when changing comment sort.

Unsure how apollo handled this, as I mainly used Infinity.

Could also add this in a later release.

That should be possible by using scrollToIndex(1) if on the post detail page!

@mdecourcy
Copy link
Contributor Author

@aeharding Thanks! Added an index var to pass-through for future use. Defaults to 0 as that will, most likely, be the primary use case.

@mdecourcy mdecourcy changed the title Feature/scroll to top on sort change Fix/scroll to top on sort change Jul 11, 2023
@mdecourcy
Copy link
Contributor Author

@aeharding does this need anything else to get merged in? I ran it last night and noticed if comment sort is changed while view is above comments, it scrolls down to index 1. Should this be changed?

@aeharding
Copy link
Owner

Hi @mdecourcy thanks so much for this PR!

I just pushed some adjustments. Mostly because of existing buggy behavior that I've wanted to get sorted for a while now.

But also, I made it so that scroll up only occurs upon new data loaded, not before. That minimizes the "jumping". I also removed the smooth scroll for this, it seemed a little less buggy.

I'm curious what you think, so please pull and try it out!

@mdecourcy
Copy link
Contributor Author

@aeharding sorry just got back from the office. After a brief look it seems like comment sort change doesn't go up to the top on long threads.

Also should the scroll up behavior on sort change jump up to the top, or mimic what happens on press of the "posts" tab?

@aeharding
Copy link
Owner

What device are you running? It seems to be going to the top on iOS with long threads

@mdecourcy
Copy link
Contributor Author

This was happening on chrome on my macbook, but after shutting down and rerunning the dev server it seems to have resolved. Looks good to me.

Going to ping you on matrix in a sec here

@aeharding aeharding merged commit cf413e9 into aeharding:main Jul 12, 2023
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.

Page Scroll Position Not Changed After Changing Post/Comment Sort
3 participants