-
Notifications
You must be signed in to change notification settings - Fork 47.1k
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
Make tests use ReactTestUtils.renderIntoDocument #1522
Conversation
@@ -1,4 +1,4 @@ | |||
/** | |||
/** |
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.
An oopsie! :)
In cases where you need the container, I think it makes the most sense to just use createElement as now; we can use renderIntoDocument for the cases where the container isn't needed. If you end up needing another step to get the node you don't gain any benefit from renderIntoDocument and we definitely shouldn't use _rootNodeID in all the tests for this. |
What @spicyj said :) Thanks for taking this on @danielschonfeld! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@danielschonfeld Any chance you can make these updates? No problem if you don't have time but if not I'll close this out. |
@spicyj I'll try to devote time to it this week but can't promise i'll get it done.... |
@danielschonfeld No problem. I'll close this out for now so it's not sitting around but if you do have time please push an update and let me know; I'm happy to reopen it. |
@danielschonfeld do you mind if i pick up on your work? |
@zpao - this PR only has about a 1/3 of the tests converted where it seemed applicable. I wanted to start a discussion about a situation I ran into multiple times and wasn't sure what the desired way was to go about.
In some tests such as in ReactCSSTransitionGroup (and many others) there's a need to perform operations on the original container that the component got mounted on. So far I've used something like
@syranide points out and rightfully so that access to private instance's ID will probably not be available in ES6 classes which brings the question of whether or not we should make a helper method for this use case or come up with a better alternative.
I'm open to feedback on this and I'm referencing issue #1250