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

Extended icon and selectedIcon propType to include scale #5131

Closed
wants to merge 1 commit into from

Conversation

cem2ran
Copy link

@cem2ran cem2ran commented Jan 5, 2016

Resolves issue #4591

Not sure what the best practice is with regards to de-duplication in propTypes, and if that proptype should therefore be declared as a variable and reused for icon and selectedIcon?

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @nicklockwood, @vjeux and @sahrens to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 5, 2016
@nicklockwood
Copy link
Contributor

Image.propTypes.source is already the canonical representation for image source shape. It doesn't currently include scale, but it probably should, along with width and height.

@cem2ran
Copy link
Author

cem2ran commented Jan 5, 2016

That was my initial thought too. I tried adding scale to an Image in ImageExample to test it out, but it didn't seem to scale:

<Image style={{
    width: 75,
    height: 75,
    backgroundColor: 'transparent',
    marginRight: 10,
  }}
  source={{uri: base64Icon, scale: 3}} />

Is this the expected behaviour? Where should I be looking to determine the missing propTypes?
This is my first deep dive into source.

@facebook-github-bot
Copy link
Contributor

@cem2ran updated the pull request.

@mkonicek
Copy link
Contributor

Image.propTypes.source is already the canonical representation for image source shape.

I tried adding scale to an Image in ImageExample to test it out, but it didn't seem to scale:

I believe we should solve that problem in a generic way as discussed above, rather than this PR. I'll close this. Let me know if I'm mistaken :)

@mkonicek mkonicek closed this Mar 20, 2016
@nicklockwood
Copy link
Contributor

@cem2ran setting scale in an <Image> source is unlikely to have any effect, as the scaling of images is controlled by the width, height and resizeMode styles.

The scale property is useful only in cases where the image size isn't specified any other way, such as the icons in a TabBarIOS.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants