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

[added] trigger onAfterOpen callback when available. #154

Merged

Conversation

diasbruno
Copy link
Collaborator

Hi,

This patch would help to sync the refs to be accessed from the Component (so, no need for a setTimeout). I've used the suggested name for the callback in #50 onAfterOpen.

This would help with #50 and #51.

Hope I followed the contribute guide.

@@ -76,6 +76,7 @@ var ModalPortal = module.exports = React.createClass({
focusManager.markForFocusLater();
this.setState({isOpen: true}, function() {
this.setState({afterOpen: true});
(this.props.isOpen && this.props.onAfterOpen) && this.props.onAfterOpen();

Choose a reason for hiding this comment

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

Why not

if(this.props.isOpen && this.props.onAfterOpen) { 
  this.props.onAfterOpen();
}

here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @victor-torres! Fixed.

@diasbruno diasbruno force-pushed the feature/onAfterOpen-callback branch 2 times, most recently from 386d662 to c0642c1 Compare April 11, 2016 13:48
@diasbruno
Copy link
Collaborator Author

@claydiffrient just pinging to see if this helps with both mentioned issues and a little review in this PR. :)

@claydiffrient
Copy link
Contributor

@diasbruno Thanks for the ping. The suggested change that @victor-torres made sounds good to me. That will keep the code a bit more readable for those in the future.

@diasbruno
Copy link
Collaborator Author

Awesome! I'll fix this.

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.

None yet

3 participants