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) - portals #1749
(fix) - portals #1749
Conversation
@@ -178,8 +178,145 @@ describe('Portal', () => { | |||
} | |||
|
|||
render(<App />, root); | |||
expect(dialog.childNodes.length).to.equal(1); | |||
expect(dialog.childNodes.length).to.equal(2); |
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.
This is because of the empty textNode, one would expect replaceNode
to delete it but for some reason it isn't deleted.
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.
Sweet 👍 💯 As said in Slack we can revisit the root handling after the X release. For now this is the best solution we could've wished for and compat
doesn't have to be as strict about size. Users that won't use Portals
won't be affected anyway, because of tree shaking 👍
Adds
1018112311410388B to the compat package 👎This is a remake of: #1713
I needed to clear my head a bit and start over I felt like I was stuck in a rut.
I have almost gotten to the fix the only remaining issue is that when we insert another portal in the same container and unmount one of them we lose both. I don't know if this is an issue or not.... Let's assume it is, working on a fix for it but it can possibly get quite complicated.
One of the options would be to check the _children of the array for an existing portal and if there is one remove from the array.... I’m not sure about this