Added support for corner radii in Android #4252

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
10 participants
@mattds

mattds commented Nov 20, 2015

This is a cut down version of a previous pull request with just the 4 corners catered for.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 20, 2015

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

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

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Nov 25, 2015

Contributor

@mkonicek is this good-to-go?

Contributor

nicklockwood commented Nov 25, 2015

@mkonicek is this good-to-go?

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Dec 24, 2015

Collaborator

@mattds Can you rebase?

Collaborator

satya164 commented Dec 24, 2015

@mattds Can you rebase?

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 31, 2015

@mattds updated the pull request.

@mattds updated the pull request.

@mattds

This comment has been minimized.

Show comment
Hide comment
@mattds

mattds Dec 31, 2015

@satya164, apologies for the delay, sure, I've rebased.

mattds commented Dec 31, 2015

@satya164, apologies for the delay, sure, I've rebased.

@satya164

This comment has been minimized.

Show comment
Hide comment
Collaborator

satya164 commented Jan 1, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 1, 2016

@mattds updated the pull request.

@mattds updated the pull request.

@mattds

This comment has been minimized.

Show comment
Hide comment
@mattds

mattds Jan 1, 2016

It's a pleasure, I updated the pull request with a minor change to getOutline of ReactViewBackgroundDrawable - now it sets mNeedUpdatePathForBorderRadius = true only if mPathForBorderRadius == null so that the subsequent call to updatePath will only update the path if it's not set or changed.

mattds commented Jan 1, 2016

It's a pleasure, I updated the pull request with a minor change to getOutline of ReactViewBackgroundDrawable - now it sets mNeedUpdatePathForBorderRadius = true only if mPathForBorderRadius == null so that the subsequent call to updatePath will only update the path if it's not set or changed.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 6, 2016

Contributor

@kmagiera Does this look OK to you?

Contributor

mkonicek commented Jan 6, 2016

@kmagiera Does this look OK to you?

@kmagiera

View changes

...Android/src/main/java/com/facebook/react/views/image/ReactImageView.java
RoundingParams roundingParams = hierarchy.getRoundingParams();
- roundingParams.setCornersRadius(hierarchyRadius);
+ roundingParams.setCornersRadii(mBorderRadius);

This comment has been minimized.

@kmagiera

kmagiera Jan 6, 2016

Contributor

You can perhaps use setCornersRadii(float topLeft, float topRight, float bottomRight, float bottomLeft) and this way have mBorderRadius to only store 4 floats

@kmagiera

kmagiera Jan 6, 2016

Contributor

You can perhaps use setCornersRadii(float topLeft, float topRight, float bottomRight, float bottomLeft) and this way have mBorderRadius to only store 4 floats

@kmagiera

View changes

...Android/src/main/java/com/facebook/react/views/image/ReactImageView.java
}
@Override
public void process(Bitmap output, Bitmap source) {
output.setHasAlpha(true);
- if (mBorderRadius < 0.01f) {
+ if (mBorderRadius[0] < 0.01f &&

This comment has been minimized.

@kmagiera

kmagiera Jan 6, 2016

Contributor

extract 0.01f as a constant

@kmagiera

kmagiera Jan 6, 2016

Contributor

extract 0.01f as a constant

@kmagiera

View changes

...roid/src/main/java/com/facebook/react/views/image/ReactImageManager.java
+ view.setBorderRadius(1, radius);
+ view.setBorderRadius(2, radius);
+ view.setBorderRadius(3, radius);
+ return;

This comment has been minimized.

@kmagiera

kmagiera Jan 6, 2016

Contributor

IMO if {} else {} construct would be much cleaner to read in this case (also less error prone for people making changes in the future, as it's easy to overlook that return statement)

@kmagiera

kmagiera Jan 6, 2016

Contributor

IMO if {} else {} construct would be much cleaner to read in this case (also less error prone for people making changes in the future, as it's easy to overlook that return statement)

@kmagiera

View changes

...roid/src/main/java/com/facebook/react/views/image/ReactImageManager.java
+ return;
+ }
+
+ view.setBorderRadius(index - 1, radius);

This comment has been minimized.

@kmagiera

kmagiera Jan 6, 2016

Contributor

So apparently this isn't correct:

React send prop updates as diffs (only sending props that have changed). Imagine you have component that has borderRadius: 10, borderTopLeftRadius: 5. You expect it to have border radius 10 everywhere except from top-left corder, where radius should be 5. Then when you update your component to have the following props: borderRadius: 20, borderTopLeftRadius: 5, react will only send update for "borderRadius" prop, which will reset also "topLeft" and will result in the component having radius set to 20 for each corner while you expect it to still have radius 5 for top-left corner

@kmagiera

kmagiera Jan 6, 2016

Contributor

So apparently this isn't correct:

React send prop updates as diffs (only sending props that have changed). Imagine you have component that has borderRadius: 10, borderTopLeftRadius: 5. You expect it to have border radius 10 everywhere except from top-left corder, where radius should be 5. Then when you update your component to have the following props: borderRadius: 20, borderTopLeftRadius: 5, react will only send update for "borderRadius" prop, which will reset also "topLeft" and will result in the component having radius set to 20 for each corner while you expect it to still have radius 5 for top-left corner

@kmagiera

View changes

...ndroid/src/main/java/com/facebook/react/views/view/ReactViewManager.java
+ view.setBorderRadius(1, radius);
+ view.setBorderRadius(2, radius);
+ view.setBorderRadius(3, radius);
+ return;

This comment has been minimized.

@kmagiera

kmagiera Jan 6, 2016

Contributor

Please refer to my comment from the similar piece of code above

@kmagiera

kmagiera Jan 6, 2016

Contributor

Please refer to my comment from the similar piece of code above

@kmagiera

View changes

...Android/src/main/java/com/facebook/react/views/image/ReactImageView.java
+ }
+
+ if (!FloatUtil.floatsEqual(mBorderRadius[position*2], radius)) {
+ mBorderRadius[position*2] = radius;

This comment has been minimized.

@kmagiera

kmagiera Jan 6, 2016

Contributor

nit: [position * 2]

@kmagiera

kmagiera Jan 6, 2016

Contributor

nit: [position * 2]

@kmagiera

This comment has been minimized.

Show comment
Hide comment
@kmagiera

kmagiera Jan 6, 2016

Contributor

@mattds Thanks for your work on this PR. Please see my comments inline. Also do you mind adding an example to UIExplorer/BorderExample.js of a component with 2 different radii + another example with elevation?

Contributor

kmagiera commented Jan 6, 2016

@mattds Thanks for your work on this PR. Please see my comments inline. Also do you mind adding an example to UIExplorer/BorderExample.js of a component with 2 different radii + another example with elevation?

@mattds

This comment has been minimized.

Show comment
Hide comment
@mattds

mattds Jan 6, 2016

@kmagiera thanks for reviewing, I will make the changes mentioned and resubmit. I see now that borderRadius will potentially overwrite corner radii as you mentioned - I think I will need to keep borderRadius around as a separate value as a default for unset corners, will revise and add the examples mentioned.

mattds commented Jan 6, 2016

@kmagiera thanks for reviewing, I will make the changes mentioned and resubmit. I see now that borderRadius will potentially overwrite corner radii as you mentioned - I think I will need to keep borderRadius around as a separate value as a default for unset corners, will revise and add the examples mentioned.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 7, 2016

@mattds updated the pull request.

@mattds updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 7, 2016

@mattds updated the pull request.

@mattds updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 7, 2016

@mattds updated the pull request.

@mattds updated the pull request.

@mattds

This comment has been minimized.

Show comment
Hide comment
@mattds

mattds Jan 7, 2016

@kmagiera, I've updated the pull request with the changes mentioned. The outline used by elevation now also accounts for the border width. Please let me know if any further changes are needed.

mattds commented Jan 7, 2016

@kmagiera, I've updated the pull request with the changes mentioned. The outline used by elevation now also accounts for the border width. Please let me know if any further changes are needed.

@kmagiera

This comment has been minimized.

Show comment
Hide comment
@kmagiera

kmagiera Jan 8, 2016

Contributor

@mattds Thanks a lot, will look at your code soon. In the meantime would you mind posting a screenshot of the new examples you've added in BorderExample

Contributor

kmagiera commented Jan 8, 2016

@mattds Thanks a lot, will look at your code soon. In the meantime would you mind posting a screenshot of the new examples you've added in BorderExample

@kmagiera

View changes

Libraries/Image/ImageStylePropTypes.js
@@ -32,6 +32,12 @@ var ImageStylePropTypes = {
// It changes the color of all the non-transparent pixels to the tintColor
tintColor: ColorPropType,
opacity: ReactPropTypes.number,
+
+ // Android-Specific styles

This comment has been minimized.

@kmagiera

kmagiera Jan 8, 2016

Contributor

Can you verify that this is indeed required here? Looks like all the properties you're adding are already a part of ViewStylePropTypes. I prefer for the way of declaring props for the views to be consistent across View and Image (that would be, just expecting it to be set in style).

I think what you should do here is to actually just get rid of these few lines added. Then for Image component you can put borderTopLeftRadius and others as a part of style and it should work with no additional changes.

Also: Can you include a screenshot of "Border Radius" example from ImageExample.js to confirm there is no regression there. Note that in the aforementioned example borderRadius is set through style property.

@kmagiera

kmagiera Jan 8, 2016

Contributor

Can you verify that this is indeed required here? Looks like all the properties you're adding are already a part of ViewStylePropTypes. I prefer for the way of declaring props for the views to be consistent across View and Image (that would be, just expecting it to be set in style).

I think what you should do here is to actually just get rid of these few lines added. Then for Image component you can put borderTopLeftRadius and others as a part of style and it should work with no additional changes.

Also: Can you include a screenshot of "Border Radius" example from ImageExample.js to confirm there is no regression there. Note that in the aforementioned example borderRadius is set through style property.

This comment has been minimized.

@nicklockwood

nicklockwood Jan 8, 2016

Contributor

Image (on iOS, at least) only supportsa subset of the border styles. It doesn't support individual corner radii or different border colors or widths for each side.

For that reason, having the styles specified separately for Image is intentional.

This diff adds additional border styles for Image on Android that aren't supported on iOS. I'm not sure what the best way to handle that is.

Maybe we could just implement these styles for View for now and not Image? I don't think there's a lot of demand for individually specified corners radii on images.

@nicklockwood

nicklockwood Jan 8, 2016

Contributor

Image (on iOS, at least) only supportsa subset of the border styles. It doesn't support individual corner radii or different border colors or widths for each side.

For that reason, having the styles specified separately for Image is intentional.

This diff adds additional border styles for Image on Android that aren't supported on iOS. I'm not sure what the best way to handle that is.

Maybe we could just implement these styles for View for now and not Image? I don't think there's a lot of demand for individually specified corners radii on images.

@kmagiera

View changes

...Android/src/main/java/com/facebook/react/views/image/ReactImageView.java
+ float bottomRightRadius = mBorderCornerRadii != null && !CSSConstants.isUndefined(mBorderCornerRadii[2]) ? mBorderCornerRadii[2] : defaultBorderRadius;
+ float bottomLeftRadius = mBorderCornerRadii != null && !CSSConstants.isUndefined(mBorderCornerRadii[3]) ? mBorderCornerRadii[3] : defaultBorderRadius;
+
+ return new float[] {

This comment has been minimized.

@kmagiera

kmagiera Jan 8, 2016

Contributor

Allocations on android are pretty expensive (mainly because of the fact they need to be garbage collected, and still lots of android devices doesn't have a decent implementation of GC).

Looks like the pattern in which you use the value returned from here is that you call cornderRadii, then almost immediately you use the returned values to pass them to some other method (the important bit is that you use values from the array as opposed to passing the reference to that array). Also note that you only call this method from the UIThread. In this case you can easily avoid allocating this array and use a static array of size 4. The preferred way to do that would be to define the array at the top of the class, then change the sigatore of cornerRadii method to take output array as an argument. In places where you call the method, you can pass that static array, then retrieve values from it.

@kmagiera

kmagiera Jan 8, 2016

Contributor

Allocations on android are pretty expensive (mainly because of the fact they need to be garbage collected, and still lots of android devices doesn't have a decent implementation of GC).

Looks like the pattern in which you use the value returned from here is that you call cornderRadii, then almost immediately you use the returned values to pass them to some other method (the important bit is that you use values from the array as opposed to passing the reference to that array). Also note that you only call this method from the UIThread. In this case you can easily avoid allocating this array and use a static array of size 4. The preferred way to do that would be to define the array at the top of the class, then change the sigatore of cornerRadii method to take output array as an argument. In places where you call the method, you can pass that static array, then retrieve values from it.

@kmagiera

View changes

...Android/src/main/java/com/facebook/react/views/image/ReactImageView.java
+
+ float[] radii = cornerRadii();
+
+ return new float[]{

This comment has been minimized.

@kmagiera

kmagiera Jan 8, 2016

Contributor

Please refer to my comment in cornerRadii method. I think the same applies here

@kmagiera

kmagiera Jan 8, 2016

Contributor

Please refer to my comment in cornerRadii method. I think the same applies here

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 8, 2016

@mattds updated the pull request.

@mattds updated the pull request.

@joelcloralt

This comment has been minimized.

Show comment
Hide comment
@joelcloralt

joelcloralt Jan 15, 2016

Contributor

Hey guys,

I just noticed borderRadious does not work on Android when the Image has child components.

This works:

<Image style={styles.imageStyle} source=... />

This also works:

<Image style={styles.imageStyle} source=... >
</Image>

This doesn't

<Image style={styles.imageStyle} source=... resizeMode="cover" >
    <View style={styles.overlayView}></View>
</Image>

Styles:

imageStyle: {
    width:75,
    height:75,
    borderRadius:5,
    overflow: 'hidden',
},
overlayView: {
    opacity:0.7,
    backgroundColor:'#333333',
    borderRadius:5,
    overflow: 'hidden',
    width:75,
    height:75,
}

Results: (Android on the left, iOS on the right) See the last image on bottom right.
screenshot 2016-01-15 10 11 30

Any ideas?

Contributor

joelcloralt commented Jan 15, 2016

Hey guys,

I just noticed borderRadious does not work on Android when the Image has child components.

This works:

<Image style={styles.imageStyle} source=... />

This also works:

<Image style={styles.imageStyle} source=... >
</Image>

This doesn't

<Image style={styles.imageStyle} source=... resizeMode="cover" >
    <View style={styles.overlayView}></View>
</Image>

Styles:

imageStyle: {
    width:75,
    height:75,
    borderRadius:5,
    overflow: 'hidden',
},
overlayView: {
    opacity:0.7,
    backgroundColor:'#333333',
    borderRadius:5,
    overflow: 'hidden',
    width:75,
    height:75,
}

Results: (Android on the left, iOS on the right) See the last image on bottom right.
screenshot 2016-01-15 10 11 30

Any ideas?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 18, 2016

Contributor

@joelcloralt can an Image have child components? I'm not sure that's going to work properly. Can you make the Image and the overlay view siblings?

Contributor

mkonicek commented Jan 18, 2016

@joelcloralt can an Image have child components? I'm not sure that's going to work properly. Can you make the Image and the overlay view siblings?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Jan 18, 2016

Contributor

@kmagiera Do you want to shipit? (@facebook-github-bot shipit)

Contributor

mkonicek commented Jan 18, 2016

@kmagiera Do you want to shipit? (@facebook-github-bot shipit)

@joelcloralt

This comment has been minimized.

Show comment
Hide comment
@joelcloralt

joelcloralt Jan 18, 2016

Contributor

@mkonicek not sure if officially supported, but how else do you make a background image with react native? If I make it a sibling it won't be on top of the image. Unless there is a flexbox trick I don't know about.

Contributor

joelcloralt commented Jan 18, 2016

@mkonicek not sure if officially supported, but how else do you make a background image with react native? If I make it a sibling it won't be on top of the image. Unless there is a flexbox trick I don't know about.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 18, 2016

Contributor

@mkonicek images with child components is fully supported on iOS, but on Android it's implemented in the JS file, by wrapping <Image> inside a <View>. That's probably why the corner radius doesn't work correctly.

Contributor

nicklockwood commented Jan 18, 2016

@mkonicek images with child components is fully supported on iOS, but on Android it's implemented in the JS file, by wrapping <Image> inside a <View>. That's probably why the corner radius doesn't work correctly.

@joelcloralt

This comment has been minimized.

Show comment
Hide comment
@joelcloralt

joelcloralt Jan 18, 2016

Contributor

mmm I just wrapped the image inside a view and border radious still doesn't work for Android

<View>
  <Image style={styles.imageStyle} source=... resizeMode="cover" >
    <View style={styles.overlayView}></View>
  </Image>
</View>
Contributor

joelcloralt commented Jan 18, 2016

mmm I just wrapped the image inside a view and border radious still doesn't work for Android

<View>
  <Image style={styles.imageStyle} source=... resizeMode="cover" >
    <View style={styles.overlayView}></View>
  </Image>
</View>
@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 18, 2016

Contributor

@joelcloralt I don't know if borderRadius clipping applies to subviews on Android - perhaps @kmagiera can confirm that?

Contributor

nicklockwood commented Jan 18, 2016

@joelcloralt I don't know if borderRadius clipping applies to subviews on Android - perhaps @kmagiera can confirm that?

@mattds

This comment has been minimized.

Show comment
Hide comment
@mattds

mattds Jan 18, 2016

@joelcloralt, one option is to position the image absolutely as the first child of a view:

<View style={{width: 100, height: 100, alignItems:'center', justifyContent: 'center'}}>
           <Image
              style={{position:'absolute', top: 0, bottom: 0, left: 0, right: 0, borderRadius:50}}
              resizeMode={'cover'}
              source={{uri: 'http://facebook.github.io/react/img/logo_og.png'}}
            />
                 <View style={{width: 20, height: 20, backgroundColor: 'white'}}/>
      </View>

mattds commented Jan 18, 2016

@joelcloralt, one option is to position the image absolutely as the first child of a view:

<View style={{width: 100, height: 100, alignItems:'center', justifyContent: 'center'}}>
           <Image
              style={{position:'absolute', top: 0, bottom: 0, left: 0, right: 0, borderRadius:50}}
              resizeMode={'cover'}
              source={{uri: 'http://facebook.github.io/react/img/logo_og.png'}}
            />
                 <View style={{width: 20, height: 20, backgroundColor: 'white'}}/>
      </View>
@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 18, 2016

Contributor

@mattds since that's basically how subviews inside <Image> are implemented on Android anyway, I'll wager you can make that work without the extra wrapper view by just debugging the code in Image.android.js - it's probably applying the border styles to the wrong element or something.

Contributor

nicklockwood commented Jan 18, 2016

@mattds since that's basically how subviews inside <Image> are implemented on Android anyway, I'll wager you can make that work without the extra wrapper view by just debugging the code in Image.android.js - it's probably applying the border styles to the wrong element or something.

@joelcloralt

This comment has been minimized.

Show comment
Hide comment
@joelcloralt

joelcloralt Jan 18, 2016

Contributor

@mattds that works perfectly. Thank you!

Contributor

joelcloralt commented Jan 18, 2016

@mattds that works perfectly. Thank you!

@mattds

This comment has been minimized.

Show comment
Hide comment
@mattds

mattds Jan 19, 2016

@joelcloralt it's a pleasure :) @nicklockwood you're right, changing

var imageProps = merge(nativeProps, {
          style: styles.absoluteImage,
          children: undefined,
        });

to

var imageProps = merge(nativeProps, {
          style: flattenStyle([styles.absoluteImage, nativeProps.style.borderRadius ? {borderRadius: nativeProps.style.borderRadius} : null]),
          children: undefined,
        });

in Image.android.js seems to work without the extra view.

mattds commented Jan 19, 2016

@joelcloralt it's a pleasure :) @nicklockwood you're right, changing

var imageProps = merge(nativeProps, {
          style: styles.absoluteImage,
          children: undefined,
        });

to

var imageProps = merge(nativeProps, {
          style: flattenStyle([styles.absoluteImage, nativeProps.style.borderRadius ? {borderRadius: nativeProps.style.borderRadius} : null]),
          children: undefined,
        });

in Image.android.js seems to work without the extra view.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jan 22, 2016

Collaborator

ping @kmagiera ;)

Collaborator

brentvatne commented Jan 22, 2016

ping @kmagiera ;)

@dcflow dcflow referenced this pull request Jan 26, 2016

Closed

Image w/ Subviews Radius! #5559

@kmagiera

This comment has been minimized.

Show comment
Hide comment
@kmagiera

kmagiera Jan 26, 2016

Contributor

@joelcloralt @nicklockwood is right. On android Image component with children is implemented in JS by wrapping children in a View with Image inside of it as a first child. Therefore corner radius won't work as you may expect as on Android we don't support children clipping based on the corner radius (see the explanation here)

Contributor

kmagiera commented Jan 26, 2016

@joelcloralt @nicklockwood is right. On android Image component with children is implemented in JS by wrapping children in a View with Image inside of it as a first child. Therefore corner radius won't work as you may expect as on Android we don't support children clipping based on the corner radius (see the explanation here)

- : 0;
- outline.setRoundRect(getBounds(), mBorderRadius + extraRadiusFromBorderWidth);
+ if((!CSSConstants.isUndefined(mBorderRadius) && mBorderRadius > 0) || mBorderCornerRadii != null) {
+ if (mPathForBorderRadiusOutline == null) {

This comment has been minimized.

@kmagiera

kmagiera Jan 26, 2016

Contributor

Do we need this check here? I think that what we should do instead is that we should set mNeedUpdatePathForBorderRadius = true in both setRadius methods to be consistent with the way how we update that property.

@kmagiera

kmagiera Jan 26, 2016

Contributor

Do we need this check here? I think that what we should do instead is that we should set mNeedUpdatePathForBorderRadius = true in both setRadius methods to be consistent with the way how we update that property.

@kmagiera

This comment has been minimized.

Show comment
Hide comment
@kmagiera

kmagiera Jan 26, 2016

Contributor

Thanks @mattds and sorry for the delay in review. I have just last one comment inline. Will also let @mkonicek merge this as every time I "ship" stuff, something breaks :)

Contributor

kmagiera commented Jan 26, 2016

Thanks @mattds and sorry for the delay in review. I have just last one comment inline. Will also let @mkonicek merge this as every time I "ship" stuff, something breaks :)

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 27, 2016

@mattds updated the pull request.

@mattds updated the pull request.

@mattds

This comment has been minimized.

Show comment
Hide comment
@mattds

mattds Jan 27, 2016

@kmagiera, no worries. I moved "mNeedUpdatePathForBorderRadius = true" to the setRadius methods, this will indeed make the check in getOutline unnecessary, thanks. I've checked in the changes. @mkonicek is this version ready to ship?

mattds commented Jan 27, 2016

@kmagiera, no worries. I moved "mNeedUpdatePathForBorderRadius = true" to the setRadius methods, this will indeed make the check in getOutline unnecessary, thanks. I've checked in the changes. @mkonicek is this version ready to ship?

@mpretty-cyro

This comment has been minimized.

Show comment
Hide comment
@mpretty-cyro

mpretty-cyro Feb 8, 2016

Contributor

@mattds Just FYI, there is a way to get something similar working on iOS by setting a layer mask (although it's performance isn't great) - https://developer.apple.com/library/ios/documentation/UIKit/Reference/UIBezierPath_class/#//apple_ref/occ/clm/UIBezierPath/bezierPathWithRoundedRect:cornerRadius:

A better write up can be found here - http://ronnqvi.st/thinking-like-a-bzier-path/

Contributor

mpretty-cyro commented Feb 8, 2016

@mattds Just FYI, there is a way to get something similar working on iOS by setting a layer mask (although it's performance isn't great) - https://developer.apple.com/library/ios/documentation/UIKit/Reference/UIBezierPath_class/#//apple_ref/occ/clm/UIBezierPath/bezierPathWithRoundedRect:cornerRadius:

A better write up can be found here - http://ronnqvi.st/thinking-like-a-bzier-path/

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Feb 8, 2016

Contributor

@mpretty-homepass this already works on iOS (using pretty-much the technique you described).

Contributor

nicklockwood commented Feb 8, 2016

@mpretty-homepass this already works on iOS (using pretty-much the technique you described).

@mpretty-cyro

This comment has been minimized.

Show comment
Hide comment
@mpretty-cyro

mpretty-cyro Feb 8, 2016

Contributor

@nicklockwood oh really? Is this something I can do via React Native? (Couldn't seem to find it in the docs).

I mostly mentioned it because of the comment about iOS support here - #4252 (comment)

Contributor

mpretty-cyro commented Feb 8, 2016

@nicklockwood oh really? Is this something I can do via React Native? (Couldn't seem to find it in the docs).

I mostly mentioned it because of the comment about iOS support here - #4252 (comment)

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Feb 8, 2016

Contributor

@mpretty-homepass yes, it's supported on iOS for everything except <Image> components, which only support equal borders and corner radii. (But you can work around that by wrapping your <Image> in another <View> with overflow:hidden).

Contributor

nicklockwood commented Feb 8, 2016

@mpretty-homepass yes, it's supported on iOS for everything except <Image> components, which only support equal borders and corner radii. (But you can work around that by wrapping your <Image> in another <View> with overflow:hidden).

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Feb 8, 2016

Contributor

@kmagiera do you want to take a look try shipit again? The plan is we'll make it work reliably this half :) Not sure all your comments have been addressed.

Contributor

mkonicek commented Feb 8, 2016

@kmagiera do you want to take a look try shipit again? The plan is we'll make it work reliably this half :) Not sure all your comments have been addressed.

@kmagiera

This comment has been minimized.

Show comment
Hide comment
@kmagiera

kmagiera Feb 8, 2016

Contributor

Yeah, let me try landing this. Looks good and all comments have been address. Thanks @mattds for your patience.

@facebook-github-bot shipit

Contributor

kmagiera commented Feb 8, 2016

Yeah, let me try landing this. Looks good and all comments have been address. Thanks @mattds for your patience.

@facebook-github-bot shipit

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment

@ghost ghost closed this in 4937a4c Feb 8, 2016

@mpretty-cyro

This comment has been minimized.

Show comment
Hide comment
@mpretty-cyro

mpretty-cyro Feb 22, 2016

Contributor

@mattds Is this meant to be working on ImageView's as well?

Contributor

mpretty-cyro commented Feb 22, 2016

@mattds Is this meant to be working on ImageView's as well?

@mattds

This comment has been minimized.

Show comment
Hide comment
@mattds

mattds Feb 22, 2016

@mpretty-homepass, so this pull request is just for regular views, #5197 is for images on Android, but since the same functionality is missing on iOS, it probably won't be merged in.

mattds commented Feb 22, 2016

@mpretty-homepass, so this pull request is just for regular views, #5197 is for images on Android, but since the same functionality is missing on iOS, it probably won't be merged in.

@mpretty-cyro

This comment has been minimized.

Show comment
Hide comment
@mpretty-cyro

mpretty-cyro Feb 22, 2016

Contributor

@mattds that's fine in iOS since you can clip child views by adding a borderRadius to their parents; but that doesn't work in Android. Which means that without this, there is no way to replicate the effect on Android.

Contributor

mpretty-cyro commented Feb 22, 2016

@mattds that's fine in iOS since you can clip child views by adding a borderRadius to their parents; but that doesn't work in Android. Which means that without this, there is no way to replicate the effect on Android.

@mpretty-cyro

This comment has been minimized.

Show comment
Hide comment
@mpretty-cyro

mpretty-cyro Feb 22, 2016

Contributor

Thanks for pointing to the other PR though, will merge with our fork and use that for the time being.

Contributor

mpretty-cyro commented Feb 22, 2016

Thanks for pointing to the other PR though, will merge with our fork and use that for the time being.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Feb 22, 2016

Contributor

I (wrongly) assumed at the time of review that subview clipping worked on Android. Given that it doesn't, I think it's probably worth revisiting #5197 - although ideally I'd like us to get this working the same on both platforms, instead of needing separate platform-specific solutions.

Contributor

nicklockwood commented Feb 22, 2016

I (wrongly) assumed at the time of review that subview clipping worked on Android. Given that it doesn't, I think it's probably worth revisiting #5197 - although ideally I'd like us to get this working the same on both platforms, instead of needing separate platform-specific solutions.

@mpretty-cyro

This comment has been minimized.

Show comment
Hide comment
@mpretty-cyro

mpretty-cyro Feb 22, 2016

Contributor

@nicklockwood I completely agree that ideally Android and iOS would function in the same way, but for the time being at least that would allow us to render the same elements on both platforms.

Contributor

mpretty-cyro commented Feb 22, 2016

@nicklockwood I completely agree that ideally Android and iOS would function in the same way, but for the time being at least that would allow us to render the same elements on both platforms.

pglotov added a commit to pglotov/react-native that referenced this pull request Mar 15, 2016

Added support for corner radii in Android
Summary:
This is a cut down version of a previous pull request with just the 4 corners catered for.
Closes facebook#4252

Reviewed By: svcscm

Differential Revision: D2911959

Pulled By: androidtrunkagent

fb-gh-sync-id: 7ddcd684d90d4d92ccefed906c0126e92818dcde

ghost pushed a commit that referenced this pull request Apr 13, 2016

mattds Facebook Github Bot 1
Added support for Image corner radii in Android
Summary: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](https://cloud.githubusercontent.com/assets/1407729/12200126/d3caceac-b625-11e5-8281-06274732a281.png)
Closes #5197

Differential Revision: D3138725

Pulled By: mkonicek

fb-gh-sync-id: df772fd07fe85386ae4c681f9e79a19d2316d38b
fbshipit-source-id: df772fd07fe85386ae4c681f9e79a19d2316d38b

ide added a commit to expo/react-native that referenced this pull request May 5, 2016

Added support for Image corner radii in Android
Summary: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](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 added a commit to expo/react-native that referenced this pull request May 6, 2016

Added support for Image corner radii in Android
Summary: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](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 added a commit to nightingale/react-native that referenced this pull request Jun 18, 2016

Added support for Image corner radii in Android
Summary: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](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 issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment