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

Moved custom scroll parent to a prop called getScrollParent #179

Merged
merged 6 commits into from
Oct 10, 2018
Merged

Moved custom scroll parent to a prop called getScrollParent #179

merged 6 commits into from
Oct 10, 2018

Conversation

k2snowman69
Copy link

Resolves #177

Basic issue is based on documentation you should not inherit from react components, instead you should override using composition.

@danbovey
Copy link
Owner

danbovey commented Oct 9, 2018

Looking at adding this feature in the v2 branch as this will be a breaking change.

I've implemented a clearer getParentElement method that can be overridden with a customParent prop.

https://github.com/CassetteRocks/react-infinite-scroller/blob/3a707215dfd2065845d0d876c853a0253d866a03/src/InfiniteScroll.js#L30-L37

README.md Show resolved Hide resolved
@k2snowman69
Copy link
Author

k2snowman69 commented Oct 10, 2018

With the way this is written currently, it's an entirely backwards compatible change. The majority of the change resides in getParentElement, the changes in attachScrollListener were mainly to reduce the number of calls to getParentElement for debugging purposes.

Digging into the backcompat scenarios:

  1. If a user doesn't pass this.props.getScrollParent scrollParent is null and will result in the original el && el.element code being executed.
  2. If a user does pass this.props.getScrollParent scrollParent then the resulting value is returned
  3. If a user inherits from this component, they would override getParentElement and none of the code within this component would be executed. If they do call super, we end in the first scenario

As far as the naming, customParent sounds like you're wrapping the InfiniteScroll component with another custom element... it's a bit confusing to me without reading the documentation.

If you are still planning on only pulling this request into the v2 branch even though it's backwards compatible, let me know, I'll have to figure something out for the short term.

@danbovey
Copy link
Owner

danbovey commented Oct 10, 2018

You're right! That's great. I'll merge this now.

I'll use the getScrollParent prop in v2 as well, as it's is more intuitive that it expects a function returning an element.

@danbovey danbovey merged commit 9c4d2e1 into danbovey:master Oct 10, 2018
@danbovey
Copy link
Owner

danbovey commented Oct 10, 2018

Released in 1.2.2. Thanks a lot!

@danbovey danbovey changed the title Moved custom scroll parent to a function prop Moved custom scroll parent to a prop called getScrollParent Oct 10, 2018
@k2snowman69
Copy link
Author

Thanks for the quick turn around!

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.

2 participants