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 onSuccess call after render call #11

Merged
merged 3 commits into from Jun 26, 2017

Conversation

Projects
None yet
2 participants
@Flave
Copy link
Contributor

Flave commented Jun 22, 2017

In order to get the dimensions (e.g. clientHeight, clientWidth) of the embed it needs to be rendered to the screen. This is why the onSuccess call is better made after the internal state update that causes the rerender.
Alternatively there could be an additional callback such as onRender. But since I didn’t see any particular advantage of placing the onSuccess call before the render, it is easier to just move it to the end. Hope this makes sense :)

Moved onSuccess call after render call
In order to get the dimensions (e.g. clientHeight, clientWidth) of the embed it needs to be rendered to the screen. This is why the onSuccess call is better made after the internal state update that causes the rerender.
Alternatively there could be an additional callback such as `onRender`. But since I didn’t see any particular advantage of placing the onSuccess call before the render, it is easier to just move it to the end. Hope this makes sense :)
@sugarshin

This comment has been minimized.

Copy link
Owner

sugarshin commented Jun 23, 2017

Thank you for this contribution!
Sounds good, but it's breaking change. that's way onRender option is good your idea. (I think onAfterRender or onRenderd is more better)
Can you fix it?

e.x

this.props.onSuccess && this.props.onSuccess(response)
this.setState(
  { __html: response.html },
  () => {
    window.instgrm.Embeds.process()
    this.props.onAfterRender && this.props.onAfterRender()
  }
)
@Flave

This comment has been minimized.

Copy link
Contributor Author

Flave commented Jun 23, 2017

Yes sounds good. I added an onAfterRender prop. I added the prop to the omitComponentProps function without knowing exactly what it does. I'm not really familiar with TypeScript (that's what this is, right?) so I hope I didn't break anything :)

@sugarshin

This comment has been minimized.

Copy link
Owner

sugarshin commented Jun 26, 2017

Thanks! I will few fix it later.

@sugarshin sugarshin referenced this pull request Jun 26, 2017

Merged

Add onAfterRender #12

@sugarshin sugarshin merged commit f1f2e7c into sugarshin:master Jun 26, 2017

@sugarshin

This comment has been minimized.

Copy link
Owner

sugarshin commented Jun 26, 2017

@Flave Merged. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment