-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
PostStream: Fix minor load more issue #2388
PostStream: Fix minor load more issue #2388
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my goodness, this issue has been driving me crazy during testing! Although I'm unsure about the placement of the onscroll
call relative to everything else: that essentially wipes out the results of using updateScrubberHeight
vs this.updateScrubber
. Perhaps it might be better to factor out the "load more" checks into a separate function like we've done with calculatePosition
and updateScrubber
so it can be called independently?
Also, you accidentially commited dist changes 😛
5485aea
to
0a9af55
Compare
When testing locally, |
This is might be due to #2384. Let's merge that in first and check again. |
We could later also check if running |
ddfd126
to
62c812c
Compare
62c812c
to
39d0acf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it again, and it works correctly with app.current.get('stream').goToIndex(19)
(yay!).
app.current.get('stream').goToIndex(20)
doesn't work (it does nothing), but that behavior is the same on master, so that's a separate issue (probably an off-by-one somewhere)
The only nitpick I have is that I think loadPostsIfNeeded
would be a better method name than loadPostsWhenNeeded
Tested it as well, and it works. Also fixed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goToIndex(20)
issue was already fixed in 09e2736, so I don't think we should duplicate it here (especially since the PR is about another topic), but otherwise LGTM!
842f180
to
d39a944
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, works as intended
When we scroll to a post that is already loaded, the load more button might show and not trigger.
To replicate (in beta.14) go to the start of a long enough discussion and run
app.current.get('stream').goToIndex(19)
.Screenshot
before the fix:
Confirmed