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

Added support for Image corner radii in Android #5197

Closed
wants to merge 1 commit into from
Closed

Added support for Image corner radii in Android #5197

wants to merge 1 commit into from

Conversation

mattds
Copy link

@mattds mattds commented Jan 8, 2016

Split out from PR #4252 - @kmagiera I've made the changes to how the radii arrays are allocated, is the approach I've taken correct? also it looks like ImageStylePropTypes are needed so I left them in for the moment. I suppose this pull request will only be valid if iOS supports image corner radii, but at least it's here if/when needed. Attached an image of how it handles the existing case:
screen shot 2016-01-08 at 4 21 25 pm

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @mkonicek, @kmagiera and @brentvatne 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 8, 2016
@mkonicek
Copy link
Contributor

I'm not convinced we should add and maintain this code in the core given it's sort of an edge-case feature, and Android only. Would you be up for releasing a separate image module that supports this for people who need this feature?

@brentvatne
Copy link
Collaborator

@mkonicek - as I mentioned in Discord, and I think you agree with me now -- this exists on iOS and web and this is a good step forward for parity.

@mattds
Copy link
Author

mattds commented Mar 21, 2016

Hi, thanks for considering this pull request, please let me know if I need to update to resolve any conflicts since I originally submitted it - also if necessary I can look to move this into separate image module as suggested.

@kmagiera
Copy link
Contributor

Would be great if you could squash and rebase your changes in top of master - so that we can see if the tests are passing

@facebook-github-bot
Copy link
Contributor

@mattds updated the pull request.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@mattds updated the pull request.

borderTopLeftRadius: ReactPropTypes.number,
borderTopRightRadius: ReactPropTypes.number,
borderBottomLeftRadius: ReactPropTypes.number,
borderBottomRightRadius: ReactPropTypes.number,
Copy link
Contributor

Choose a reason for hiding this comment

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

@brentvatne how is this being used on iOS? If it does exists there, why do we need to add "android-specific" code here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry @kmagiera, it seems that I was mistaken here: these properties are not currently supported directly on the Image component on iOS. The only way to re-create this effect on iOS is by adding a corner radius to a wrapper View, but this does not work on Android.

<View style={styles.test}>
  <Image
    style={styles.image}
    source={{uri: 'https://www.petfinder.com/wp-content/uploads/2012/11/140272627-grooming-needs-senior-cat-632x475.jpg'}}
  />
</View>
test: {
  borderTopLeftRadius: 20,
  overflow: 'hidden',
},

Results in:

screen shot 2016-03-25 at 12 18 03 pm

On the web, and on React Native Android with this PR, assigning the border property specifically to images works:

screen shot 2016-03-25 at 12 18 31 pm

So even though this isn't actually the current behaviour on iOS I think it is still desirable for us to pull this in, and we can bring iOS to parity eventually. cc @vjeux

@kmagiera
Copy link
Contributor

Just a few minor comments inline and it's good to go. Thanks @mattds for your patience :)

@mattds
Copy link
Author

mattds commented Mar 22, 2016

It's a pleasure, will make the changes mentioned now.

@facebook-github-bot
Copy link
Contributor

@mattds updated the pull request.

@kmagiera
Copy link
Contributor

Looks good, just waiting for @brentvatne response on that inline comment

@vjeux
Copy link
Contributor

vjeux commented Mar 25, 2016

Thanks, this is going to be very useful :) borderRadius was kind of limited as is.

@ghost
Copy link

ghost commented Mar 27, 2016

@mkonicek would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

@mkonicek
Copy link
Contributor

mkonicek commented Apr 1, 2016

@brentvatne replied: #5197 (comment)

Will let @kmagiera review this one.

@kmagiera
Copy link
Contributor

kmagiera commented Apr 5, 2016

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 69534a3 Apr 13, 2016
ide pushed a commit to expo/react-native that referenced this pull request May 5, 2016
Summary:Split out from PR facebook#4252 - kmagiera I've made the changes to how the radii arrays are allocated, is the approach I've taken correct? also it looks like ImageStylePropTypes are needed so I left them in for the moment. I suppose this pull request will only be valid if iOS supports image corner radii, but at least it's here if/when needed. Attached an image of how it handles the existing case:
![screen shot 2016-01-08 at 4 21 25 pm](https://cloud.githubusercontent.com/assets/1407729/12200126/d3caceac-b625-11e5-8281-06274732a281.png)
Closes facebook#5197

Differential Revision: D3138725

Pulled By: mkonicek

fb-gh-sync-id: df772fd07fe85386ae4c681f9e79a19d2316d38b
fbshipit-source-id: df772fd07fe85386ae4c681f9e79a19d2316d38b
ide pushed a commit to expo/react-native that referenced this pull request May 6, 2016
Summary:Split out from PR facebook#4252 - kmagiera I've made the changes to how the radii arrays are allocated, is the approach I've taken correct? also it looks like ImageStylePropTypes are needed so I left them in for the moment. I suppose this pull request will only be valid if iOS supports image corner radii, but at least it's here if/when needed. Attached an image of how it handles the existing case:
![screen shot 2016-01-08 at 4 21 25 pm](https://cloud.githubusercontent.com/assets/1407729/12200126/d3caceac-b625-11e5-8281-06274732a281.png)
Closes facebook#5197

Differential Revision: D3138725

Pulled By: mkonicek

fb-gh-sync-id: df772fd07fe85386ae4c681f9e79a19d2316d38b
fbshipit-source-id: df772fd07fe85386ae4c681f9e79a19d2316d38b
zebulgar pushed a commit to nightingale/react-native that referenced this pull request Jun 18, 2016
Summary:Split out from PR facebook#4252 - kmagiera I've made the changes to how the radii arrays are allocated, is the approach I've taken correct? also it looks like ImageStylePropTypes are needed so I left them in for the moment. I suppose this pull request will only be valid if iOS supports image corner radii, but at least it's here if/when needed. Attached an image of how it handles the existing case:
![screen shot 2016-01-08 at 4 21 25 pm](https://cloud.githubusercontent.com/assets/1407729/12200126/d3caceac-b625-11e5-8281-06274732a281.png)
Closes facebook#5197

Differential Revision: D3138725

Pulled By: mkonicek

fb-gh-sync-id: df772fd07fe85386ae4c681f9e79a19d2316d38b
fbshipit-source-id: df772fd07fe85386ae4c681f9e79a19d2316d38b
This pull request was closed.
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.

6 participants