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

fixed issue with non-scrollable results list #3888

Closed
wants to merge 3 commits into from
Closed

Conversation

fsw
Copy link

@fsw fsw commented Oct 30, 2015

In a scenario when one page of ajax results fits into results area, it becomes non-scrollable and "loading more results.." is displayed at the bottom, is unusable and there is no way to load more results:

http://jsfiddle.net/fz7q66pe/3/

This seems to be an edge-case but we had our results being displayed in a large modal and in a grid so this became a problem on large screens.

This fix triggers scroll event after results are appended and it will check if "load more" is already visible and take proper action.

In a scenario when one page of ajax results fits into results area, it becomes non-scrollable and "load more" button is displayed at the bottom, is unusable and there is no way to load more results:

http://jsfiddle.net/fz7q66pe/2/

This seems to be an edge-case but we had our results being displayed in a large modal and in a grid so this became a problem on large screens.

This fix triggers scroll event after results are appended and it will check if "load more" is already visible and take proper action.
@alexweissman
Copy link
Contributor

@fsw I realize that you submitted this PR over 2 years ago, but do you still stand by it as a solution? I'm worried that manually triggering the scroll event could have other, unintended side effects.

@fsw
Copy link
Author

fsw commented Nov 22, 2017

@alexweissman I am not sure if I stand by it because I can not even recall creating this PR :)
I think that project I used this in is using this fix till now and no bugs were reported.

But indeed calling JS event here seems harsh and could have side effects.

I guess that extracting a callback from here:
https://github.com/fsw/select2/blob/b7fc8f5cc764fc3b9e2ed2675dc81d88cb4b4bc3/src/js/select2/dropdown/infiniteScroll.js#L40
into a method and calling it "loadMoreIfLoadingMoreVisible"
and calling this method somehow instead of a callback would be a lot cleaner.

So this one does the trick but could be done better.

cheers.

@alexweissman
Copy link
Contributor

That's exactly what I was thinking!

@fsw
Copy link
Author

fsw commented Nov 25, 2017

I guess this looks better now. I tested this manually and works fine on my fiddle.
Is there anything more I can do here? Should I commit build dist in this PR as well?

@alexweissman
Copy link
Contributor

Nope, no need to (and actually should not) commit any dist/ code. I will run the build process and make the changes in dist/ when this PR is merged.

A qunit test for this edge case would be helpful though, if possible!

@diakonos
Copy link

Buuuuump. 🤜🏼🤛🏼

I just had to fix this in our local version of Select2. I was about to open an issue to begin the process of getting this properly fixed for Select2 as a whole, but then I found this PR!

Any reason why this never got merged?

I checked out the changes, and I'm actually not sure if it will solve all cases. In my case, I needed to trigger the loadMoreIfLoadingMoreVisible function for both results:append as well as results:all because the append event isn't triggered for the first render, and in our case we needed it checked at this point as well.

@fsw
Copy link
Author

fsw commented May 15, 2018

@diakonos I was testing this with ajax so results were empty initially. You are probably right re results:all.

filip-diligentia added a commit to Diligentia-Uitgeverij/select2 that referenced this pull request Jun 14, 2018
fix paging in infinte scroll. code stolen from select2#3888 thx
@stale
Copy link

stale bot commented Mar 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label Mar 13, 2019
@stale stale bot removed the status: stale label Mar 19, 2019
@kevin-brown kevin-brown removed this from the 4.0.6 milestone Apr 28, 2019
@stale
Copy link

stale bot commented Jun 27, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label Jun 27, 2019
@stale stale bot closed this Jul 4, 2019
kevin-brown added a commit that referenced this pull request Jul 20, 2019
Ever since the 4.0.0 release of Select2, there has been a bug where
if you enabled infinite scrolling but did not return enough results
on the first load of AJAX to show a scrollbar, then infinite
scrolling would not be enabled and you could not view anything other
than the first page of results. The solution for this was first
proposed in #3888 but it was closed off because of inactivity and
missing tests.

This fixes the issue by performing the check to see if more results
should be loaded both on scroll and also when the results are first
loaded. This solves the issue that we were seeing before, because
the plugin knows it needs to load in more results, just it did not
receive the scroll event before and thus was not able to actually
load in the new results.

This has the potential to trigger multiple AJAX requests to load in
multiple pages of results if the user has the ability to see many
options, but only a few are being loaded at a time.

This also adds tests for infinite scrolling, both to ensure that
it will attempt to load additional pages, even without the scrollbar,
and to ensure that the regular behaviour of not loading additional
pages when the scrollbar is visible is preserved.

Fixes #3088
kevin-brown added a commit that referenced this pull request Jul 20, 2019
Ever since the 4.0.0 release of Select2, there has been a bug where
if you enabled infinite scrolling but did not return enough results
on the first load of AJAX to show a scrollbar, then infinite
scrolling would not be enabled and you could not view anything other
than the first page of results. The solution for this was first
proposed in #3888 but it was closed off because of inactivity and
missing tests.

This fixes the issue by performing the check to see if more results
should be loaded both on scroll and also when the results are first
loaded. This solves the issue that we were seeing before, because
the plugin knows it needs to load in more results, just it did not
receive the scroll event before and thus was not able to actually
load in the new results.

This has the potential to trigger multiple AJAX requests to load in
multiple pages of results if the user has the ability to see many
options, but only a few are being loaded at a time.

This also adds tests for infinite scrolling, both to ensure that
it will attempt to load additional pages, even without the scrollbar,
and to ensure that the regular behaviour of not loading additional
pages when the scrollbar is visible is preserved.

Fixes #3088
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants