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

Loader with custom scrollable never fires #126

Closed
nandorstanko opened this issue Jan 5, 2016 · 6 comments
Closed

Loader with custom scrollable never fires #126

nandorstanko opened this issue Jan 5, 2016 · 6 comments

Comments

@nandorstanko
Copy link

@mariuszzak
I had to change the following lines in https://github.com/hhff/ember-infinity/blob/master/addon/components/infinity-loader.js to make this work how I expected:

_selfOffset() {
    if (this.get('_customScrollableIsDefined')) {
      return this.$().position().top + this.get("_scrollable").scrollTop();
    } else {
      return this.$().offset().top;
    }
  },

Maybe I'm missing something, but adding this.get("_scrollable").scrollTop() to the top position is unnecessary. Returning only this.$().position().top, in case when there is a custom scrollable item defined, is enough. In the current code _selfOffset is always bigger than _bottomOfScrollableOffset, if the loader is not in the view.

@davidgoli
Copy link
Collaborator

@hhff This is a regression that broke with 0.2.1

@hhff
Copy link
Collaborator

hhff commented Jan 11, 2016

yup. @mariuszzak can you take a look here?

@mdentremont
Copy link
Contributor

One thing that did change (which affected us) was _checkIfInView was renamed to _loadMoreIfNeeded (v0.2.0...v0.2.1)

@jimmay5469
Copy link
Contributor

I believe I have found the issue... this.$().position().top in the snippet above is making an assumption that the _scrollable element is the direct parent of the loader which may or may not be the case. What I believe we need is this.$().offset().top - this.get("_scrollable").offset().top which does not make any assumptions. I will try to create a PR shortly and make sure all the tests continue to pass with the change.

@hhff
Copy link
Collaborator

hhff commented Aug 5, 2016

thats @jimmay5469 !

yup correct - it's been on my todo list to fix for sometime now... I would really love a PR for it!

@jimmay5469
Copy link
Contributor

PR submitted! Hopefully it helps!

@hhff hhff closed this as completed in #173 Aug 5, 2016
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

No branches or pull requests

5 participants