Skip to content

[added] trigger onAfterOpen callback when available. - #154

Merged
claydiffrient merged 1 commit into
reactjs:masterfrom
diasbruno:feature/onAfterOpen-callback
Apr 12, 2016
Merged

[added] trigger onAfterOpen callback when available.#154
claydiffrient merged 1 commit into
reactjs:masterfrom
diasbruno:feature/onAfterOpen-callback

Conversation

@diasbruno

Copy link
Copy Markdown
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.

Comment thread lib/components/ModalPortal.js Outdated
focusManager.markForFocusLater();
this.setState({isOpen: true}, function() {
this.setState({afterOpen: true});
(this.props.isOpen && this.props.onAfterOpen) && this.props.onAfterOpen();

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Collaborator Author

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

@claydiffrient

Copy link
Copy Markdown
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
Copy Markdown
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.

3 participants