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

Update type check for NavigationCardStack #9808

Conversation

lgan1989
Copy link
Contributor

@lgan1989 lgan1989 commented Sep 8, 2016

The typecheck inside of NavigationCard is PropTypes.any while in NavigationCardStack it is View.propTypes.style.

let's make them consistent to avoid unnecessary warnings. (e.g. trying to pass a animationStyle as cardStyle)

let's make them consistent to avoid unnecessary warnings.
(e.g. trying to pass a animationStyle into cardStyle)
@ghost
Copy link

ghost commented Sep 8, 2016

By analyzing the blame information on this pull request, we identified @hedgerwang and @jmurzy to be potential reviewers.

@ghost
Copy link

ghost commented Sep 8, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 8, 2016
@ghost
Copy link

ghost commented Sep 8, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@mkonicek
Copy link
Contributor

mkonicek commented Sep 8, 2016

The typecheck inside of NavigationCard is PropTypes.any while in NavigationCardStack it is View.propTypes.style.

It seems like View.propTypes.style is more accurate? Why not use it in both places? PropTypes.any is an equivalent of no typing.

let's make them consistent to avoid unnecessary warnings. (e.g. trying to pass a animationStyle as cardStyle)

What is animationStyle and why is not a View.propTypes.style?

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 8, 2016
@lgan1989
Copy link
Contributor Author

lgan1989 commented Sep 8, 2016

@mkonicek the thing is that NavigationCard's render() is using a Animated.View the style property could be as following:

<Animated.View style={{opacity: this.state.fadeAnim}}> {this.props.children} </Animated.View>

while
this.state = { fadeAnim: new Animated.Value(0), };

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 8, 2016
@ericvicenti
Copy link
Contributor

Ah, I see the problem. Can't we use Animated.View.propTypes.style?

@lgan1989
Copy link
Contributor Author

lgan1989 commented Sep 9, 2016

@ericvicenti this could work, although after I changed it to Animated.View.propTypes.style it exposed another issue(warning):

since there is a property in NavigationPropTypes named "position", and it's conflicted with the ViewStylePropTypes check inside of AnimatedImplementation.js@1485.

Although I think this false warning is not critical and it's because the checking itself in AnimatedImplementation is not so precise.

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 9, 2016
@satya164
Copy link
Contributor

since there is a property in NavigationPropTypes named "position", and it's conflicted with the ViewStylePropTypes check inside of AnimatedImplementation.js@1485.

I have had this issue with one of my modules and I think PropTypes.any is our best best for now.

@ghost
Copy link

ghost commented Sep 14, 2016

@lgan1989 updated the pull request - view changes

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 14, 2016
@ghost
Copy link

ghost commented Sep 22, 2016

@lgan1989 updated the pull request - view changes

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 22, 2016
@facebook-github-bot
Copy link
Contributor

@lgan1989 updated the pull request - view changes

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 17, 2016
@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Dec 5, 2016
@facebook-github-bot
Copy link
Contributor

@ericvicenti has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

robclouth pushed a commit to robclouth/react-native that referenced this pull request Dec 7, 2016
Summary:
The typecheck inside of NavigationCard is PropTypes.any while in NavigationCardStack it is  View.propTypes.style.

let's make them consistent to avoid unnecessary warnings. (e.g. trying to pass a animationStyle as cardStyle)
Closes facebook#9808

Differential Revision: D4277323

Pulled By: ericvicenti

fbshipit-source-id: c30b4a21675cad98c19f5c6522e286d776bfa20d
mkonicek pushed a commit that referenced this pull request Dec 12, 2016
Summary:
The typecheck inside of NavigationCard is PropTypes.any while in NavigationCardStack it is  View.propTypes.style.

let's make them consistent to avoid unnecessary warnings. (e.g. trying to pass a animationStyle as cardStyle)
Closes #9808

Differential Revision: D4277323

Pulled By: ericvicenti

fbshipit-source-id: c30b4a21675cad98c19f5c6522e286d776bfa20d
DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
Summary:
The typecheck inside of NavigationCard is PropTypes.any while in NavigationCardStack it is  View.propTypes.style.

let's make them consistent to avoid unnecessary warnings. (e.g. trying to pass a animationStyle as cardStyle)
Closes facebook#9808

Differential Revision: D4277323

Pulled By: ericvicenti

fbshipit-source-id: c30b4a21675cad98c19f5c6522e286d776bfa20d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants