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

Component should update when className is changed #36

Merged
merged 1 commit into from
Jul 4, 2017

Conversation

WendellLiu
Copy link
Contributor

it will make this component flexible to make some style change

(flexible to make some style change)
@Treora
Copy link
Collaborator

Treora commented Sep 2, 2016

Looks good to me, I can't think of cases where this would harm.

As I wrote yesterday:

perhaps we should think about a more general way to check for changes in props that are passed through to the element. Should the component perhaps update if any prop has changed, except if our tests find that the change is already reflected in the DOM?

I would suggest to merge this for now, and possibly later we can try out if updating on every prop change not related to innerHTML would indeed be an improvement.

@eugene1g
Copy link

eugene1g commented Sep 9, 2016

In the same spirit, it would be great to reflect changes in style prop as well.

@monolithed
Copy link

monolithed commented Jan 24, 2017

It can be implemented something like:

{
	shouldComponentUpdate (nextProps) {
		let { props, htmlEl } = this;

		// We need not rerender if the change of props simply reflects the user's edits. 
		// Rerendering in this case would make the cursor/caret jump.

		// Rerender if there is no element yet... (somehow?)
		if (!htmlEl) {
			return true;
		}

		// ...or if html really changed... (programmatically, not by user edit)
		if (nextProps.html !== htmlEl.innerHTML && nextProps.html !== props.html) {
			return true;
		}

		let optional = ['style', 'className', 'disable'];

		// Handle additional properties
		return optional.some(name => props[name] !== nextProps[name]);
	}
}

@ajbeaven
Copy link
Contributor

ajbeaven commented Jul 4, 2017

Can this be merged please?

@Treora
Copy link
Collaborator

Treora commented Jul 4, 2017

I get the impression nobody is actively maintaining this project anymore.. Neither am I, but I can still press the merge button if that helps anyone. I am not able to publish a release update however, I guess you may have to kindly poke @lovasoa for that.

@Treora Treora merged commit 932596b into lovasoa:master Jul 4, 2017
lovasoa added a commit that referenced this pull request Jul 4, 2017
ajbeaven added a commit to ajbeaven/react-contenteditable that referenced this pull request Jul 4, 2017
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.

5 participants