-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Remove componentWillReceiveProps usages in examples #1952
Remove componentWillReceiveProps usages in examples #1952
Conversation
Hi @Chaitanya-Deep, welcome to Draft.js and thanks for taking the time to look into this issue! This change looks good to me 👍🏼, sounds good to remove the sketchy |
} | ||
|
||
componentDidMount() { | ||
this._update(); | ||
} | ||
|
||
componentWillReceiveProps(nextProps) { | ||
if (nextProps.content !== this.props.content) { | ||
componentDidUpdate(prevProps) { |
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.
Maybe worth calling out there's an unused prevState
second argument here.
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.
Made the change but is that necessary?
The third snapshot
argument is also unused.
And the docs for typical usage of componentDidUpdate don't include it either
https://reactjs.org/docs/react-component.html#componentdidupdate
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.
@claudiopro is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
…x/deprecated_react_lifecycle_methods
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.
@niveditc has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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.
Thanks for working on this! Requesting changes to split the gulp native update into a separate PR.
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.
Looks good! :)
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.
@niveditc has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
…1952) Summary: **Summary** Resolves [1940](facebookarchive#1940) Change usage of `componentWillReceiveProps` in Tex examples to `componentDidUpdate` Pull Request resolved: facebookarchive#1952 Reviewed By: claudiopro Differential Revision: D13417818 Pulled By: claudiopro fbshipit-source-id: b13ff3140c3207cddaeb8d98c239f7dfd4b04a47
Summary: **Summary** Resolves [1940](facebookarchive/draft-js#1940) Change usage of `componentWillReceiveProps` in Tex examples to `componentDidUpdate` Pull Request resolved: facebookarchive/draft-js#1952 Reviewed By: claudiopro Differential Revision: D13417818 Pulled By: claudiopro fbshipit-source-id: b13ff3140c3207cddaeb8d98c239f7dfd4b04a47
Summary: **Summary** Resolves [1940](facebookarchive/draft-js#1940) Change usage of `componentWillReceiveProps` in Tex examples to `componentDidUpdate` Pull Request resolved: facebookarchive/draft-js#1952 Reviewed By: claudiopro Differential Revision: D13417818 Pulled By: claudiopro fbshipit-source-id: b13ff3140c3207cddaeb8d98c239f7dfd4b04a47
Summary
Resolves 1940
Change usage of
componentWillReceiveProps
in Tex examples tocomponentDidUpdate