Skip to content

allow scrolling props.parent instead of on window only#10

Merged
KyleAMathews merged 1 commit into
KyleAMathews:masterfrom
astrails:master
Nov 27, 2015
Merged

allow scrolling props.parent instead of on window only#10
KyleAMathews merged 1 commit into
KyleAMathews:masterfrom
astrails:master

Conversation

@borisnadion

Copy link
Copy Markdown

@KyleAMathews

Copy link
Copy Markdown
Owner

Cool! Curious what your use case is? Also why checkin the compiled code?

@KyleAMathews

Copy link
Copy Markdown
Owner

Hey would love to commit this. Please just remove the checked-in dist stuff.

@KyleAMathews

Copy link
Copy Markdown
Owner

Also, another question—why make parent a function? Does the parent element change often with react-infinite?

@borisnadion

Copy link
Copy Markdown
Author

Updated ;-) No, it's not changed.

@KyleAMathews

Copy link
Copy Markdown
Owner

Thanks! And if you could squash it down to one commit as well everything
looks great now.
On Sat, Jul 25, 2015 at 2:55 PM Boris Nadion notifications@github.com
wrote:

Updated ;-) Not it's not changed.


Reply to this email directly or view it on GitHub
#10 (comment)
.

@borisnadion

Copy link
Copy Markdown
Author

done

@borisnadion

Copy link
Copy Markdown
Author

parent has to be a function, we should bind a listener to an existing dom node

@KyleAMathews

Copy link
Copy Markdown
Owner

Why does parent have to be a function?

@KyleAMathews

Copy link
Copy Markdown
Owner

@borisnadion bumping this, would love to commit this. Why again does parent need to be a function? Why can't it just be the actual DOM node?

@borisnadion

Copy link
Copy Markdown
Author

< ScrollableContent ref="scrollable" ...>
...
</ ScrollableContent>
< Headroom parent={-> @refs.scrollable?.getDOMNode() || window}>...</ Headroom >

because the parent, or actually just source DOM element for measurements can actually be not parent at all, for example position=fixed elements inside scrollable ones look bad on iOS Safari: they're scrolled with a content and then jump to maintain fixed position when scroll is ended

@WickyNilliams

Copy link
Copy Markdown

Allows you to treat the element as a deferred value, i guess.

Is there a danger of memory leaks due to not cleaning up your event handlers, if the element you listen to can change at any moment?

Comment thread src/index.cjsx

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Parent should always be a function right? so React.PropTypes.func?

@KyleAMathews

Copy link
Copy Markdown
Owner

Cool :) makes sense. If you could rebase and address the question I raised, let's get this merged and in a new release.

@WickyNilliams

Copy link
Copy Markdown

Did you see my comment above regarding memory leaks and event listeners?

@KyleAMathews

Copy link
Copy Markdown
Owner

@WickyNilliams I did—not sure I understand—if the parent element is removed—wouldn't the event listeners get GCed?

@WickyNilliams

Copy link
Copy Markdown

Older IE will (React supports back to IE8). But I guess that's not such a priority these days haha. Stuck in my old ways :)

@arielsalminen

Copy link
Copy Markdown

Is there anything you need help with? I’d also need this feature in a project I’m working on. :-)

@KyleAMathews KyleAMathews merged commit b5e282e into KyleAMathews:master Nov 27, 2015
@KyleAMathews

Copy link
Copy Markdown
Owner

Released 1.7.0 w/ this PR included! Thanks @borisnadion!

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.

4 participants