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

[Slide] Add test coverage for componentDidMount() #6679

Closed

Conversation

agamrafaeli
Copy link
Contributor

No description provided.

@muibot muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Apr 23, 2017
instance = mountWrapper.instance();
instance.componentDidMount();
assert.strictEqual(
ReactDOM.findDOMNode(mountWrapper.instance().transition).style.transform,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for saving me the time of finding out what this is all about. Regardless it is a bad styled line. I'll work on this. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I had that issue some time ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Read the entire thread, both on the React repo and our internal issue. Not sure how to overcome this. What if findDOMNode returns Text here? Why would that happen? Should I throw an error then?

Copy link
Member

Choose a reason for hiding this comment

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

What about ignoring that issue? It's testing after all, how cares to be safe?
We could create a $FlowIgnore type for that case: https://flow.org/en/docs/config/options/#toc-suppress-type-string.

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise, we could be using if (X instanceof HTMLElement) but that might fails with some old browser through browserstack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put in a change for usage of instanceof. If it doesn't fail that is best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Reallllllyyy weird behaviour. Will look into creating our own $FlowIgnore

@oliviertassinari
Copy link
Member

@agamrafaeli This one is tough, some weird fails 😆 !

@oliviertassinari oliviertassinari force-pushed the next branch 3 times, most recently from 7cf8d3c to e4800de Compare April 30, 2017 22:12
@muibot muibot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 4, 2017
@muibot muibot added PR: Needs Review and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels May 11, 2017
}

return 'translate3d(0, 0, 0)';
// direction === 'down'
Copy link
Member

@oliviertassinari oliviertassinari May 21, 2017

Choose a reason for hiding this comment

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

I have cherry-picked that changed into #6911, good catch 👍 !
But I'm closing this PR as has been inactive for quite some time. The test is inconsistent across the different browser. I'm not sure what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! I actually knew I needed to deal with it and didn't get around. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants