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

Parse DOM after changing imagesMaxWidth #83

Closed
wants to merge 1 commit into from

Conversation

SoundBlaster
Copy link
Contributor

This commit improve componentDidUpdate behaviour and changing imagesMaxWidth prop causes a new render cycle through a DOM. It is useful for responsive HTML component. For example, if device orientation is changed it may causes an update of imagesMaxWidth value from parent component (as a result of onLayout callback) and if HTML uses custom renderers its internal logic requires update of this.state.dom value to trigger new parseDOM.

This commit improve `componentDidUpdate` behaviour and changing `imagesMaxWidth` prop causes a new render cycle through a DOM. It is useful for responsive `HTML` component. For example, if device orientation is changed it may causes an update of `imagesMaxWidth` value from parent component (as a result of `onLayout` callback) and if `HTML` uses custom `renderers` its internal logic requires update of `this.state.dom` value to trigger new `parseDOM`.
@SoundBlaster
Copy link
Contributor Author

Hi, please give a feedback to this changes.

@Exilz
Copy link
Contributor

Exilz commented Jan 29, 2018

Hi, sorry for the late response, I completely skipped both of your PRs.
This is interesting, but I wonder if it should be merged this way.

The thing is that some people reported the fact that the rendered HTML doesn't update when a re-render should be triggered (your example, hot reloading...).

I guess there should be a better solution than checking for each of these particular conditions to trigger the re-render whenever it's needed.

If I, or someone else, cannot fix this soon, I'll consider merging this. What do you think ?

@SoundBlaster
Copy link
Contributor Author

SoundBlaster commented Jan 29, 2018

Hello!
I think, what if parent of <HTML> changes an imagesMaxWidth prop then DOM must to be recalculated, because you can not predict changes inside a page layout. This prop is never changed a lot of times per component lifecycle, I guess. This commit can not degrade a performance of rendering, if you worry about it. Thanks.

@baoanhng
Copy link

baoanhng commented Feb 5, 2018

Interesting, I didn't think of orientation. But seemingly this should work together with prop's new value.

#89

@Exilz
Copy link
Contributor

Exilz commented Feb 7, 2018

I took some time to investigate this, and I came up with a solution that triggers a clean re-render after any prop change.
This means hot reloading will now work when changing your tags styles, but this also fixes imagesMaxWidth.

See #95

@Exilz Exilz mentioned this pull request Feb 8, 2018
@Exilz Exilz closed this in #95 Feb 8, 2018
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