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

Fix an issue, that would cause tinymce to be updated too often #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marchbnr
Copy link

If the props have nested elements, the comparison between the existing props and the new props will fail, due to TinyMCE doing modifications on the react component's config prop.

@tuarrep
Copy link

tuarrep commented Aug 31, 2016

Seems to not working for me. The component is still rerendered

@@ -51,7 +51,6 @@ const TinyMCE = React.createClass({
},

componentDidMount() {
const config = clone(this.props.config);
this._init(config);

Choose a reason for hiding this comment

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

should be this.props.config

Choose a reason for hiding this comment

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

I think it would be better to use cloneDeep here?

@valoricDe
Copy link

Hi, I created a merge request, incorporating your changes. Could you give your five cents on this one?: #44

@marchbnr
Copy link
Author

marchbnr commented Oct 6, 2016

I am not working on React at the moment and I don't have an environment to test your changes.

However, the problem that I had was, that the original property object was modified by TinyMCE's init, which is why I passed the deeply cloned config to the init method. That way, changing the isEqual method was not required.

It seems to me like there was a bug in the componentDidMount method, which you have fixed, but I guess that should have been enough?

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.

4 participants