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

expose updateImageState to parent components #15

Merged
merged 1 commit into from
Feb 23, 2016
Merged

expose updateImageState to parent components #15

merged 1 commit into from
Feb 23, 2016

Conversation

theolampert
Copy link
Contributor

adds a separate updateImageState to allow a parent component to trigger an image reload

@paulstraw
Copy link
Contributor

This seems fine to me, but I'm not an expert in React idioms. @frederickfogerty?

@frederickfogerty
Copy link
Contributor

@theolampert

What are your goals with this change? I'm assuming you want it to recalculate the styles based on a change in width/height? To cover that use case, I'm looking at (and would prefer) the use of DOM Mutation Observers, which pick up these changes without having to call updateImgState directly.

Am I right in thinking the intended api for this is this.refs.Imgix.updateImgState()? If it's for the use case described above, I think a name like forceLayout() or recalculateStyles() would be much more fitting.

In terms of whether I think this is an acceptable PR, I think yes. Even though we will implement Observers (as above), I think this is still necessary, perhaps for older browsers or other scenarios.

Please let me know if I'm completely on the wrong course.

@theolampert
Copy link
Contributor Author

@frederickfogerty yes you're correct - I ran into an issue at work where we needed this, sorry for not being clearer above. I think forceLayout() works well, I'll update the pr if you're happy with that.

@frederickfogerty
Copy link
Contributor

Okay cool. If you can squash your commits with a useful commit message I'm happy to merge. There's going be some issues here with unmounted components but I'm going to open a seperate issue for that, as it's not directly related to this PR.

@frederickfogerty frederickfogerty self-assigned this Feb 22, 2016
@theolampert
Copy link
Contributor Author

@frederickfogerty commits are squashed and renamed now.

frederickfogerty added a commit that referenced this pull request Feb 23, 2016
expose updateImageState to parent components
@frederickfogerty frederickfogerty merged commit fa79d23 into imgix:master Feb 23, 2016
@frederickfogerty
Copy link
Contributor

Merged and released in v2.2.0

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