-
Notifications
You must be signed in to change notification settings - Fork 13
Updated the APIs to be more swifty #34
Conversation
| - Returns: A 'UIImage' with the specified color, size and corner radius. | ||
| */ | ||
| public class func imageFromColor(_ color: UIColor, size: CGSize, cornerRadius: CGFloat) -> UIImage? { | ||
| public class func from(color: UIColor, size: CGSize, cornerRadius: CGFloat) -> UIImage? { |
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.
Is it too crazy to make this a convenience initialiser instead of a class function? Think it would make more sense.
| */ | ||
| public static func viewWithNibNamed<T>(_ nibName:String) -> T? { | ||
| let view = UINib(nibName: nibName, bundle: nil).instantiate(withOwner: nil, options: nil).first as? T | ||
| public static func fromNib<T>(_ name:String) -> T? { |
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.
What about from<T>(nibWithName: String) -> T?? Feels a bit more expressive.
| - returns: a new CGRect with the width and height reversed to those of the current one | ||
| */ | ||
| public func rectByReversingSize() -> CGRect { | ||
| public var reversedSize: CGRect { |
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.
This sounds a bit odd, as if it was return a size instead of CGRect. Is reverseSized grammatically incorrect and/or too weird? :D
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.
What about reversingSize? That seems to be similar to the examples here https://github.com/apple/swift-evolution/blob/master/proposals/0005-objective-c-name-translation.md func bezierPathByReversingPath() -> UIBezierPath -> func reversing() -> UIBezierPath
| - returns: the range between the start of the first substring and the end of the last substring | ||
| */ | ||
| public func rangeFromString(_ string: String, toString: String, searchType: RangeSearchType = .leftToRight, inRange: Range<Index>? = nil) -> Range<Index>? { | ||
| public func rangeFrom(string: String, toString: String, searchType: RangeSearchType = .leftToRight, inRange: Range<Index>? = nil) -> Range<Index>? { |
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 am pretty sure this one should be range(from fromString: String, to toString: String, .... :)
|
@chriscombs @NickSkull What's the status on this PR? Should I make those changes and merge to the swift-3.0 branch, so we can take it further from there? |
|
@mariusc Yeah, I think that would be the best, if @chriscombs agrees. |
|
@NickSkull @chriscombs Can you guys look over this and give a bit of feedback? Or merge the PR? 😄 |
| */ | ||
| public func rectByReversingSize() -> CGRect { | ||
| public var reversingSize: CGRect { | ||
| return CGRect(origin: self.origin, size: CGSize(width: self.height, height: self.width)) |
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.
Can we lose the selfs here? :D
| case resize = "resize" | ||
| case crop = "crop" | ||
| case fit = "fit" | ||
| case standard = "default" |
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.
This should be kept as default, you can do this by writing default in backticks.
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.
👍
|
Wow I don't even remember starting this PR 😄 |
| - returns: a new CGRect with the width and height reversed to those of the current one | ||
| */ | ||
| public func rectByReversingSize() -> CGRect { | ||
| public var reversingSize: CGRect { |
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.
Should change this to reversedSize
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.
It was reversedSize, but then this discussion happened #34 (comment). But I can change it back to reversedSize if you think it's better
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.
Oh, I missed that. I guess it's fine then, if that's what Apple does. Also, why is that BezierPath thing a func and not a var?
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.
You're the native speaker @chriscombs, so if you think it should be different let's do it that way. Apple is being classic in not obeying their own rules :D
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 think you're right in that reversedSize implies that it returns the size and not the rect.
| case resize = "resize" | ||
| case crop = "crop" | ||
| case fit = "fit" | ||
| case standard = "default" |
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.
👍
In line with the changes to many native APIs in swift 3.0. Some of them were fairly straightforward, but would like feedback on some of the more drastic changes.