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

Add Portal support #1263

Merged
merged 3 commits into from
Nov 15, 2017
Merged

Add Portal support #1263

merged 3 commits into from
Nov 15, 2017

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Oct 14, 2017

I’m happy to add tests, but i don’t really understand the test suite nor where the appropriate place to add them is for the v16 only feature.

@aweary from what I can tell the Portal fiber is similar to a host component in that it’s essentially a transparent wrapper over it’s rendered children. It does hold some host container info which may be required for actual DOM traversal but in terms of the RST tree this should give an accurate representation as React sees it.

@alansouzati
Copy link

Hey folks, any news here? I really want to enable tests for my React portal components 👍

@louisscruz
Copy link
Contributor

Are any efforts being made to provide portal support? This seems to be the final bit in migrating our code base (and its Enzyme tests) to React 16.

@marudor
Copy link

marudor commented Nov 6, 2017

I've merged master into this PR and tests seem to work.
Could you rebase to master and see if it passes?

@ljharb
Copy link
Member

ljharb commented Nov 6, 2017

@jquense please rebase PRs from the command line when updating them, so as to avoid merge commits :-)

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks; this is a good start! We’d definitely need tests for it; see the enzyme-test-suite dir for guidance.

@jquense
Copy link
Contributor Author

jquense commented Nov 6, 2017

@ljharb Sorry, it's not that i'm unsure how/where to write a test, but how to write ones that sufficiently test the change to ya'lls satisfaction. A single test that does a find through a portal will exercise the code path, but not particularly aggressively. If ya'll ok with none of the other traversal methods hitting this that seems fine, if not i'm not sure you want me to rewrite every test to include a portal either.

thanks for the review!

@ljharb
Copy link
Member

ljharb commented Nov 6, 2017

I think that the simple test you describe would be the minimum of what we'd want; ideally, we'd write tests for all the traversal methods, to ensure that none of them throw.

@louisscruz
Copy link
Contributor

@jquense Are you planning on writing the test? If not, I could take a crack at it.

@jquense
Copy link
Contributor Author

jquense commented Nov 8, 2017

I'm planning on it but a bit busy, happy to get some help :)

@ljharb ljharb self-assigned this Nov 14, 2017
@ljharb ljharb merged commit 7f1099d into enzymejs:master Nov 15, 2017
ljharb added a commit to ljharb/enzyme that referenced this pull request Nov 15, 2017
jquense pushed a commit to jquense/enzyme that referenced this pull request Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants