Skip to content
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

fix: update node contains conditional check on Tooltip component #988

Merged
merged 2 commits into from
Mar 23, 2023

Conversation

danielbarion
Copy link
Member

closes #985

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dargmuesli Jonas Thelemann
@gabrieljablonski
Copy link
Member

gabrieljablonski commented Mar 22, 2023

I have no problem with doing this change but it feels unnecessary. mutation.removedNodes should never have a null node inside it, and if a node is not null, it will always have the contains() method defined.

Maybe in older browsers something weird might happen? Or maybe if the reference to the node becomes stale contains becomes null?

Again, we can do the change just in case, but if #985 wasn't just a suggestion, and @rolandpoulter actually ran into an error because of this, I'd love to see a reproducible example purely out of curiosity.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
dargmuesli Jonas Thelemann
Co-authored-by: Gabriel Jablonski <gabriel.g.jablonski@gmail.com>
@danielbarion
Copy link
Member Author

danielbarion commented Mar 22, 2023

@gabrieljablonski I do agree with you, can you handle this PR and this issue, please?

@gabrieljablonski
Copy link
Member

@danielbarion No problem. I'll just wait for a response from @rolandpoulter and take it from there.

@rolandpoulter
Copy link

@gabrieljablonski I can look into the reason why, but I did get an error after upgrading to the latest version. I patched it in my node_modules with the above change because I noticed in the code you are checking for node to be truthy before calling contains elsewhere. There might be something on my end with how I wrapped the tooltip that causes this issue but it will take some time to investigate and provide the exact reproduction.

@gabrieljablonski
Copy link
Member

@gabrieljablonski I can look into the reason why, but I did get an error after upgrading to the latest version. I patched it in my node_modules with the above change because I noticed in the code you are checking for node to be truthy before calling contains elsewhere. There might be something on my end with how I wrapped the tooltip that causes this issue but it will take some time to investigate and provide the exact reproduction.

@rolandpoulter No worries, just wanted to make sure this was what solved your problem. We'll merge this change and release a new version soon.

Thanks for reporting it!

@gabrieljablonski gabrieljablonski merged commit 09f1c45 into master Mar 23, 2023
@gabrieljablonski gabrieljablonski deleted the fix/node-contains branch March 23, 2023 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when calling node.contains because it is not a method
3 participants