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

PostStream: Fix minor load more issue #2388

Merged
merged 5 commits into from
Feb 10, 2021

Conversation

w-4
Copy link
Contributor

@w-4 w-4 commented Oct 13, 2020

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:
Screenshot_316

Confirmed

  • Frontend changes: tested on a local Flarum installation.

Copy link
Member

@askvortsov1 askvortsov1 left a 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 😛

@w-4 w-4 force-pushed the wk/poststream-trigger-onscroll branch from 5485aea to 0a9af55 Compare October 13, 2020 15:45
@askvortsov1
Copy link
Member

When testing locally, app.current.get('stream').goToIndex(19) on a long discussion with these changes results in repeated scrolling to and from the scrolled post.

@w-4
Copy link
Contributor Author

w-4 commented Oct 14, 2020

This is might be due to #2384. Let's merge that in first and check again.

@w-4
Copy link
Contributor Author

w-4 commented Oct 14, 2020

We could later also check if running app.current.get('stream').goToNumber(20) works. But I'd also wait for #2378 to be merged. It removes the m.route.set call in DiscussionPage postionChanged that could lead to an additional goToNumber call.

@askvortsov1 askvortsov1 added this to the 0.1.0-beta.15 milestone Oct 15, 2020
@askvortsov1 askvortsov1 removed this from the 0.1.0-beta.15 milestone Oct 27, 2020
Copy link
Member

@askvortsov1 askvortsov1 left a 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

@w-4 w-4 requested a review from askvortsov1 December 12, 2020 05:33
@w-4
Copy link
Contributor Author

w-4 commented Dec 12, 2020

Tested it as well, and it works. Also fixed the app.current.get('stream').goToIndex(20).

Copy link
Member

@askvortsov1 askvortsov1 left a 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!

js/src/forum/states/PostStreamState.js Outdated Show resolved Hide resolved
@w-4 w-4 requested a review from askvortsov1 January 10, 2021 07:10
Copy link
Member

@askvortsov1 askvortsov1 left a 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

@askvortsov1 askvortsov1 merged commit 03231b2 into flarum:master Feb 10, 2021
@askvortsov1 askvortsov1 added this to the 0.1.0-beta.16 milestone Feb 10, 2021
matteocontrini pushed a commit to fibraclick/flarum-framework that referenced this pull request Feb 18, 2021
KyrneDev pushed a commit that referenced this pull request Feb 20, 2021
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.

3 participants