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

Exposing onHeightReady hook #43

Merged
merged 9 commits into from
Apr 14, 2016
Merged

Exposing onHeightReady hook #43

merged 9 commits into from
Apr 14, 2016

Conversation

nutgaard
Copy link
Contributor

For when you need to animate scroll or elements outside of the collapse-component.

Don't know if this is the best solution, or even if it should be part of this library. But it did allow me to add scroll-animation alongside the expanding-component.

var isOpenedChanged = isOpened !== this.props.isOpened;
this.setState({isOpenedChanged}, () => {
if (this.props.onHeightReady && isOpenedChanged) {
this.props.onHeightReady(isOpened ? this.state.height : 0);
Copy link
Owner

Choose a reason for hiding this comment

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

I think doing "side-effect" (notifying parent) in componentWillReceiveProps is dangerous. It also would potentially end up in recursion.

Potentially this could be added to some other lifecycle method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into using componentDidUpdate instead. Shouldn't really be a problem

@nkbt
Copy link
Owner

nkbt commented Apr 10, 2016

Thanks a lot for your PR. I guess it might be helpful to notify about current height, though I need to do some more testing with it.

Can you please, add an example of how you use it? That would help a lot to understand reasoning better for everyone (me included 😄)

@codecov-io
Copy link

Current coverage is 52.20%

Merging #43 into master will decrease coverage by -1.13% as of c885f2b

@@            master    #43   diff @@
=====================================
  Files           10     11     +1
  Stmts          135    159    +24
  Branches         0      0       
  Methods          0      0       
=====================================
+ Hit             72     83    +11
  Partial          0      0       
- Missed          63     76    +13

Review entire Coverage Diff as of c885f2b

Powered by Codecov. Updated on successful CI builds.

@nutgaard
Copy link
Contributor Author

Fixed the issues highlighted in the initial review, and added an example of a potential usecase, autoscrolling, see example 7 and Hooks.js.

For simplicity I've included a devDependency for the scrolling.

@nkbt nkbt merged commit c37033a into nkbt:master Apr 14, 2016
@nkbt
Copy link
Owner

nkbt commented Apr 14, 2016

Sorry for the delay, was not able to check it properly

@nutgaard
Copy link
Contributor Author

No problem. Just happy that it got merged. :)

Let me know if there is anything else you need me to do.

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.

3 participants