FeatureHighlight - migrate to new api, remove unsafe method #1290
Conversation
| const target = nextProps?.getTarget?.(); | ||
| const node = FeatureHighlight.findTargetNode(target); | ||
| if (node && node !== prevState?.node) { | ||
| return {...prevState, node}; |
There was a problem hiding this comment.
Why setting all '...prevState' and not only 'node'? What else has changed?
There was a problem hiding this comment.
contentPosition, it works without setting it, but any future state change will be reset in prop change, no?
There was a problem hiding this comment.
Once a prop changed and getDerivedStateFromProps is called you'll get the last state in 'prevState' parameter. Think of this function as setState(), you don't need to set the whole state object again, only the part you want to change as a result of a prop change. When you don't return anything from this function, the default is 'null'. Returning null doesn't reset the state, it doesn't touch it...
| } | ||
|
|
||
| componentDidUpdate() { | ||
| componentDidUpdate(nextProps) { | ||
| if (!_.isEqual(nextProps, this.props)) { |
There was a problem hiding this comment.
Is it really necessary to check all the props? All of them are relevant for setting the target position?
Inbal-Tish
changed the title
| } | ||
|
|
||
| componentDidUpdate() { | ||
| shouldSetTargetPosition = (nextProps) => { |
There was a problem hiding this comment.
Looks like 'shouldComponentUpdate'... If it's not a pure component you can just implement that method instead and the component will not be updated if it doesn't meet the condition.
There was a problem hiding this comment.
I've tried it before and now again, it's not quite working.
the position of the FeatureHighlight view is getting the previous data, that's why I went with this solution.
(I even tried !_.isEqual(nextProps, this.props))
|
@mendyEdri Where do we stand with this PR? |
Fixed what needed, you can review it :) |
Description
Replaced UNSAFE
componentWillReceivePropsmethod withgetDerivedStateFromPropsChangelog
Infra: FeatureHighlight migrated to newer React api, replaced
UNSAFEmethod withgetDerivedStateFromProps