-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
add support for borderTopLeftRadius and the three other one-corner-on… #1871
add support for borderTopLeftRadius and the three other one-corner-on… #1871
Conversation
…ly border radius props to ReactImageManager
{ | ||
view.CornerRadius = new CornerRadius(radius); | ||
var cornerRadius = view.CornerRadius; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
view.CornerRadius [](start = 31, length = 17)
Can view.CornerRadius
ever be null? You can see ReactViewManager
uses this expression:
var cornerRadius = border.CornerRadius == null ? new CornerRadius() : border.CornerRadius;
``` #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke with Yury. CornerRadius
is a struct so it can't be null and a null check is not needed.
In reply to: 197945901 [](ancestors = 197945901)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs an example added to RNTester.
/// <param name="radius">The border radius value.</param> | ||
[ReactProp("borderRadius")] | ||
public void SetBorderRadius(Border view, double radius) | ||
[ReactPropGroup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modify WPF implementation to match as well. test prop in Playground and provide code+screenshot for both UWP and WPF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthargett I have another PR ready to be submitted which fixes a bug in borderRadius and which has WPF part updated too. Is it okay if you approve this PR and I will do the Playground screenshots and code as part of the next PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to see a contribution upstream for a border*Radius
example in RNTester. Does not need to be done for this repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReactNative already has examples of border*Radius at https://github.com/facebook/react-native/edit/master/RNTester/js/BorderExample.js see styles border6, border9, border10, border14.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - but not for Image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Enum values correspond to positions of prop names in ReactPropGroup attribute | ||
/// applied to <see cref="SetBorderRadius(Border, int, double)"/> | ||
/// </summary> | ||
private enum Radius |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BEGIN NITPICK
In general I don't know why we add private enums when ints work just fine.
END NITPICK
…ly border radius props to ReactImageManager