-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add Editable placeholders #756
Conversation
|
||
const content = this.getContent(); | ||
|
||
this.setState( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably shouldn't be calling this all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this would trigger a re-render if the value is the same?
This performs a shallow merge of stateChange into the new state.
https://facebook.github.io/react/docs/react-component.html#setstate
I do notice a lot of re-renders form node change though, not from this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this would trigger a re-render if the value is the same?
Yes it does: https://codepen.io/aduth/pen/mmXgxQ
We'd need to expose and use React.PureComponent
for it to not: https://facebook.github.io/react/docs/react-api.html#react.purecomponent
Alternatively just compare against state here and return;
early if it would end up being the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great and it's simple enough 👍
If it needs any changes, a new ticket can be opened. |
Thank you so much for working on this! I know it's been quite hellish — really appreciate you sticking with it! |
:) Yeah it wasn't really adding the placeholders specifically, should have just done it like this a while ago... Let's look at the rest separately. |
This is #475 but without any other improvements.