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

Append portal node to container instead of prepend #1971

Merged
merged 4 commits into from
Oct 8, 2019

Conversation

toraora
Copy link
Contributor

@toraora toraora commented Oct 3, 2019

This fixes the issue described in #1806. Expected test outcomes were
generated by taking the test code and running it with React instead of
Preact.

This fixes the issue described in preactjs#1806. Expected test outcomes were
generated by taking the test code and running it with React instead of
Preact.
@coveralls
Copy link

coveralls commented Oct 3, 2019

Coverage Status

Coverage remained the same at 99.767% when pulling 14e2593 on toraora:master into a8c4c66 on preactjs:master.

@@ -452,6 +452,6 @@ describe('Portal', () => {

toggle();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sanity's sake could you duplicate this test with <Modal open={open}>Hello</Modal> as firstChild, I remember trying to avoid append for some reason if that works out I'll test something myself and else approve the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the test

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for taking your time to tackle this

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on my end 👍 Thank you so much for the PR 🎉

@JoviDeCroock JoviDeCroock merged commit 5c5af5a into preactjs:master Oct 8, 2019
cristianbote pushed a commit that referenced this pull request Oct 8, 2019
* Append portal node to container instead of prepend

This fixes the issue described in #1806. Expected test outcomes were
generated by taking the test code and running it with React instead of
Preact.

* Add additional test for portals testing ordering
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants