fix(Tooltip): Support for multiple target elements #1465
Conversation
|
Up |
TheSharpieOne
changed the title
| } | ||
|
|
||
| document.removeEventListener('click', this.handleDocumentClick, true) | ||
| } | ||
|
|
||
| updateTarget() { | ||
| const newTarget = getTarget(this.props.target); | ||
| if (newTarget !== this._target) { | ||
| const newTarget = getTarget(this.props.target, true); |
There was a problem hiding this comment.
This PR broke support for refs as target prop. When props.target is a React ref or an HTMLElement, this will return an HTMLElement, not an array. Array.from(newTarget) will then return an empty array, and Popper will error that target was set to undefined.
| const newTarget = getTarget(this.props.target); | ||
| if (newTarget !== this._target) { | ||
| const newTarget = getTarget(this.props.target, true); | ||
| if (newTarget !== this._targets) { |
There was a problem hiding this comment.
Using reference equality here to compare two arrays doesn't look right
There was a problem hiding this comment.
Yes, it seems like this would always be true since we would be getting a new array from getTarget every time. Probably need to perform a deep comparison here.
There was a problem hiding this comment.
And then only remove the event listeners from the targets that actually got removed (vs removing from all the targets if one was removed)
There was a problem hiding this comment.
it might be easier and quicker to just remove them all and add the newTarget back 🤷♂
It fixes #1185
Here is stackblitz demo with fix - https://stackblitz.com/edit/react-k8rkzv