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

Clear scroll context also when empty page was received #1660

merged 1 commit into from Sep 5, 2019

Clear scroll context also when empty page was received #1660

merged 1 commit into from Sep 5, 2019


Copy link

@mjanser mjanser commented Sep 5, 2019

No description provided.

Copy link

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Could you add a changelog entry?

I wonder if this could also have some side effects? Perhaps you could share some more background info on how you stumbled over this and what problem it solves?

Scroll contexts should always be cleared when they're not used
anymore. Retrieving an empty page from a scroll means it's finished
and the context can be cleared.
Copy link
Contributor Author

mjanser commented Sep 5, 2019

Added a changelog entry.

The scroll context was already cleared when a foreach loop over the scroll was finished (currentPage >= $totalPages). This change clears the context also when the page contains an empty result. This can happen if there aren't any hits and therefore the first page contains no results. This directly ends the foreach loop and the scroll context was kept open.

Copy link

ruflin commented Sep 5, 2019

Thanks for the additional details. I must confess I have not used this part of the code enough myself to fully understand the potentially side effects but it looks good. So let's get it in, especially as it doesn't break any tests :-)

Thanks for the contribution.

@ruflin ruflin merged commit 9990f64 into ruflin:master Sep 5, 2019
@mjanser mjanser deleted the fix-scroll-clear branch September 5, 2019 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants