-
Notifications
You must be signed in to change notification settings - Fork 79
Conversation
Uses better Swift 3 naming for the `path` methods.
@@ -32,7 +32,7 @@ public extension UIFont { | |||
|
|||
- returns: A Source Sans Pro `UIFont` at the specified size and weight. | |||
*/ | |||
public static func sourceSansProFontOfSize(_ fontSize: CGFloat, weight: FontWeight = .regular) -> UIFont { | |||
public static func sourceSansProFont(ofSize fontSize: CGFloat, weight: FontWeight = .regular) -> UIFont { |
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.
We can remove the word font
from these methods, no?
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.
Which font
do you propose removing? The one from sourceSansProFont
or fontSize
?
I was deliberately keeping these in line with ’s:
public class func systemFont(ofSize fontSize: CGFloat) -> UIFont
But I see how you could make an argument similar to UIColor.whateverColor()
→ UIColor.whatever()
.
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.
But I see how you could make an argument similar to UIColor.whateverColor() → UIColor.whatever().
This is exactly how I’m thinking of it. I think we can safely remove both, but if we want to be a bit more conservative just the first one.
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.
So just to be clear, you’re thinking:
public static func sourceSansPro(ofSize: size CGFloat, weight: FontWeight = .regular) -> UIFont
public static func menloRegular(ofSize size: CGFloat) -> UIFont
With call sites looking like:
UIFont.sourceSansPro(ofSize: 32.0)
UIFont.menloRegular(ofSize: 16.0)
I’d be happy to make the change but am worried we’re deviating from the naming changes in UIFont
. UIFont
APIs were renamed, from e.g.:
public class func systemFontOfSize(fontSize: CGFloat, weight: CGFloat) -> UIFont
to:
public class func systemFont(ofSize fontSize: CGFloat, weight: CGFloat) -> UIFont
with no dropping of "font".
At this stage, would a radar be more appropriate (perhaps we can call out specifics in the API guidelines to warrant these APIs changing), or do you think it’s not worth keeping directly in sync with similar signatures in UIKit
?
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.
Yeah, I guess you’re right. There is one difference UIFont.system
wouldn’t make a lot of intuitive sense because system
doesn’t really sound like a specific font, whereas UIFont.futura
definitely reads like UIColor.red
. I think a radar for now and leaving it as is a good approach. Let me know if you want me to file it.
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’m happy to take care of it.
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.
And good point about system
. I go back and forth on UIFont.system
, personally. In context, I think it would seem reasonable on its own: label.font = .system
, but with a parameter, not so much: label.font = .system(ofSize: 32)
.
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.
Filed rdar://27250975.
Begins work on API renaming as part of #123
📝 This is pointing at the new
swift-3.0
branch, notdevelop
.These are all manual edits made to API names (and a bit of refactoring where appropriate). That is to say, none of this work was performed by the migrator. Migration occurred in #127.
Related: