Update state.text if props.children change. #9
Conversation
src/index.js
Outdated
| @@ -34,6 +34,11 @@ class EllipisWithTooltip extends React.Component { | |||
| } | |||
| } | |||
|
|
|||
| componentWillReceiveProps(nextProps) { | |||
| if (nextProps.children === this.props.children) return; | |||
There was a problem hiding this comment.
it compares the references. if the values are the same do we need to update the state?
There was a problem hiding this comment.
Yes, and how do you recommend doing a value comparison here? Deep object comparison would potentially be inefficient.
src/index.js
Outdated
| @@ -34,6 +34,11 @@ class EllipisWithTooltip extends React.Component { | |||
| } | |||
| } | |||
|
|
|||
| componentWillReceiveProps(nextProps) { | |||
There was a problem hiding this comment.
componentWillReceiveProps is deprecated, how about using getDerivedStateFromProps or componentDidUpdate, instead?
There was a problem hiding this comment.
Yeah, I noticed that. I'll try getDerivedStateFromProps. I'll try to get to this tomorrow.
There was a problem hiding this comment.
@amirfefer it looks like react-ellipsis-with-tooltip depends on react 15 or 16. getDerivedStateFromProps isn't introduced until 16.3. How do you want to handle that?
There was a problem hiding this comment.
I've added support for react 16.3 :)
would you mind to rebase?
There was a problem hiding this comment.
@amirfefer synced with upstream and updated to use getDerivedStateFromProps
There was a problem hiding this comment.
hmm. node 12 test failing. investigating.
license
Outdated
| @@ -1,9 +0,0 @@ | |||
| MIT License | |||
There was a problem hiding this comment.
There are two of these. Uppercase LICENSE and lowercase license. Thought I would keep the uppercase, but I can bring it back if this was the wrong choice.
Fix for #8 including test reproducing the problem.