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

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

Merged
merged 1 commit into from
Nov 27, 2015

Conversation

borisnadion
Copy link

@KyleAMathews
Copy link
Owner

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

@KyleAMathews
Copy link
Owner

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

@KyleAMathews
Copy link
Owner

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

@borisnadion
Copy link
Author

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

@KyleAMathews
Copy link
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 [email protected]
wrote:

Updated ;-) Not it's not changed.


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

@borisnadion
Copy link
Author

done

@borisnadion
Copy link
Author

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

@KyleAMathews
Copy link
Owner

Why does parent have to be a function?

@KyleAMathews
Copy link
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
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

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?

@@ -17,6 +17,7 @@ module.exports = React.createClass
ticking: false

propTypes:
parent: React.PropTypes.node
Copy link
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
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

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

@KyleAMathews
Copy link
Owner

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

@WickyNilliams
Copy link

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

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
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