Added support for in element scrolling #17

merged 4 commits into from Oct 11, 2012


None yet
3 participants

saiwong commented Oct 4, 2012

Added support for infinite scrolling for elements with overflow set to
auto. This is enabled via the options hash by setting
useElementScroll to true

Sai Wong added some commits Oct 4, 2012

Sai Wong Added support for in element scrolling
Added support for infinite scrolling for elements with overflow set to
auto. This is enabled via the `options` hash by setting
`useElementScroll` to `true`
Sai Wong Fixed runtime error due to typo d20c1b8

reissbaker commented Oct 10, 2012

Works, but I think some improvements could be made.

  1. To remove a lot of the conditional logic, instead of listView.parent existing only under certain conditions, listView.$scrollParent could exist always. If it should scroll within a containing element, $scrollParent is set to that element; otherwise it's set to $window. Then all scrolling-related references to $window could be replaced with $scrollParent, and the code would trivially work for both.
  2. Is there a way that this could happen automatically, instead of with an option flag? It seems possible to walk up the DOM tree from the ListView element and check the overflow-y property of each containing element. If they're all visible, the ListView should use $window for scrolling; if one is auto or scroll it should use that containing element. If it's hidden, god knows what you do -- seems silly to use Infinity at all in that case, but I guess just use the containing element to save CPU cycles. inherit would be a slightly more annoying case, but you just check the parent's overflow-y and use that for the current element.



saiwong commented Oct 10, 2012

I think the $scrollParent suggestion is good and I can add that in.

For auto-detecting what to use, I think that only opens it up for issues with fringe cases that we can probably never fully enumerate. It makes sense to for the integration code to explicitly specify it so that there is no assumptions made. Less magic = better!


reissbaker commented Oct 10, 2012

Fair enough. I may still give auto-detection a shot in the future, but a configuration option that forces it to be one way or another is still a useful thing even in a world where auto-detection works. Add in the $scrollParent and I'll merge it in!


saiwong commented Oct 11, 2012

Added $scrollParent to commit!


reissbaker commented Oct 11, 2012

Excellent! Merging in.

@reissbaker reissbaker added a commit that referenced this pull request Oct 11, 2012

@reissbaker reissbaker Merge pull request #17 from saiwong/element_scrolling
Added support for in element scrolling

@reissbaker reissbaker merged commit adac94a into airbnb:master Oct 11, 2012

Should the boundViews.push(listView) call be moved into this conditional?


saiwong replied Oct 26, 2012

I don't see harm in doing so, but this is really an internal implementation and a listView should not be attached twice.

I think this is a typo, this should be a call to .off('scroll') instead of .on('scroll')


saiwong commented Nov 7, 2012

You are right. You should submit a fix for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment