Skip to content

[GENERAL] [BUGFIX] [Image] Fix props width, height and tintColor#19281

Closed
brunolemos wants to merge 2 commits intofacebook:masterfrom
brunolemos:patch-2
Closed

[GENERAL] [BUGFIX] [Image] Fix props width, height and tintColor#19281
brunolemos wants to merge 2 commits intofacebook:masterfrom
brunolemos:patch-2

Conversation

@brunolemos
Copy link
Copy Markdown
Contributor

@brunolemos brunolemos commented May 16, 2018

Test Plan

Currently this already works with the live version:

  • <View width={100} height={100} margin={100} />
  • <Image width={100} height={100} source={[{ uri: '' }]} /> // width and height when source is an array
  • <Image margin={100} /> // any style as prop, except tintColor

This only works after this pr:

  • <Image width={100} height={100} source={{ uri: '' } /> // width and height when source is not an array
  • <Image tintColor="#0000FF" /> // tintColor as prop
<Image
  width={200}
  height={200}
  borderRadius={100}
  margin={20}
  source={{ uri: 'https://arcweb.co/wp-content/uploads/2016/10/react-logo-1000-transparent.png' }}
  // style={{ width: 300, height: 300, tintColor: 'purple' }}
  tintColor="green"
/>

Release Notes

[GENERAL] [BUGFIX] [Image] Fix props width, height and tintColor

@brunolemos brunolemos requested a review from shergin as a code owner May 16, 2018 05:25
@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 May 16, 2018
@react-native-bot react-native-bot added ✅Test Plan Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels May 16, 2018
@brunolemos brunolemos changed the title Fix width, height and tintColor props on Image [GENERAL] [BUGFIX] [Image] Fix props width, height and tintColor May 16, 2018
@brunolemos
Copy link
Copy Markdown
Contributor Author

Ping @shergin @TheSavior this is a small one

@elicwhite
Copy link
Copy Markdown
Member

Hmm.

We recommend putting those props in the style prop. React Native originally shipped supporting setting styles as props instead of in the styles object but we are moving away from that. We actually expect to be removing that functionality in a future release, forcing those styles to be defined in style.

If you set them in style instead does the functionality you expect work as-is?

@brunolemos
Copy link
Copy Markdown
Contributor Author

brunolemos commented Jun 1, 2018

@TheSavior Thanks. Yes, style works as expected.

I understand this is not the recommended way of doing it, but while RN still haven't moved away from it, I see no harm on including this fix, right? Like, all other props work, except for these three.

@elicwhite
Copy link
Copy Markdown
Member

elicwhite commented Jun 1, 2018

Cool, glad style works.

We are actively removing this functionality, so I don't think we want to reverse direction on this and open the door to it being used more.

@elicwhite elicwhite closed this Jun 1, 2018
@brunolemos
Copy link
Copy Markdown
Contributor Author

brunolemos commented Jun 1, 2018

We are actively removing this functionality

@TheSavior can you point me to PRs or discussions about this? I wasn't able to find.

@elicwhite
Copy link
Copy Markdown
Member

elicwhite commented Jun 1, 2018

I don't actually think we've posted about this anywhere yet (you're hearing it first!) :'(

We have been working on rewriting the core of React Native (project is called Fabric) and want to be stricter with what props get passed in to native components. There are some perf wins to be gained here as well. One of the other problems we've had with styles being set as props are that if you define a prop on a component with the name of a style then you can run into conflicts.

We have been flow typing the core components and disallowing unsupported props being passed in that way as a first step. To support open source better we need to get a dev mode runtime warning or something out so people can find and have time to update.

Edit: Apparently there has been a TODO on these lines for the last 3 years.

@brunolemos
Copy link
Copy Markdown
Contributor Author

@TheSavior I'm removing the "styles as props" support from TypeScript definition (DefinitelyTyped/DefinitelyTyped#28217), please let me know if this is still the plan or if someone decided that it will keep working as today on Fabric.

@elicwhite
Copy link
Copy Markdown
Member

We still intend to remove styles as top level props and the React Native flow definitions enforce it as such. It would be good to have typescript be consistent with that. Thanks for making the change to the typescript definition!

@brunolemos
Copy link
Copy Markdown
Contributor Author

Ok! Thanks for the answer.

@brunolemos
Copy link
Copy Markdown
Contributor Author

TypeScript change merged and deployed.

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. Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants