-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
Refactored TRCircularProgressLayer to swift #228
base: master
Are you sure you want to change the base?
Refactored TRCircularProgressLayer to swift #228
Conversation
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.
Looking good! Left a few suggestions. Mind uploading a before/after gif to make sure the animation is still as expected?
|
||
@implementation TRCircularProgressLayer | ||
|
||
@dynamic progress, 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.
Wow, this probably shouldn’t have been here!
import UIKit | ||
|
||
@objc(TRCircularProgressLayer) class CircularProgressLayer: CALayer { | ||
@objc var progress: CGFloat = CGFloat(0) |
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 could be either : CGFloat = 0
or = CGFloat(0)
(I usually prefer the former).
override init() { | ||
super.init() | ||
self.actions = [ | ||
"bounds": NSNull(), |
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.
Curious, does a nil
literal work here? AFAIK nil
should be bridged to NSNull
, but I’m not sure if that works for CAAction
.
} | ||
|
||
override func draw(in ctx: CGContext) { | ||
let center = CGPoint(x: self.bounds.width / 2.0, y: self.bounds.height / 2.0) |
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 that in order to match the previous behaviour of TRCGPointMakeIntegral()
, these should be round()
ed. Maybe a CGPoint
extension with a func integral()
that returns a new point with each dimension rounded?
|
||
override func draw(in ctx: CGContext) { | ||
let center = CGPoint(x: self.bounds.width / 2.0, y: self.bounds.height / 2.0) | ||
let progress = min(self.progress, 1.0 - CGFloat.ulpOfOne) |
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 this could drop the explicit CGFloat
and become .ulpOfOne
.
ctx.setStrokeColor(UIColor.clear.cgColor) | ||
ctx.addArc( | ||
center: center, | ||
radius: CGFloat(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.
Is radius
already a CGFloat
?
radius: CGFloat(radius), | ||
startAngle: 0.0, | ||
endAngle: 2 * .pi, | ||
clockwise: false |
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.
The previous version of this specified clockwise as YES
.
progressPath.move(to: center) | ||
progressPath.addArc( | ||
center: center, | ||
radius: CGFloat(self.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.
Already CGFloat
?
progressPath.addArc( | ||
center: center, | ||
radius: CGFloat(self.radius), | ||
startAngle: 3.0 * .pi, |
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 was previously 3.0f * (CGFloat)M_PI_2
, or 1.5 * .pi
.
|
||
override class func needsDisplay(forKey key: String) -> Bool { | ||
switch key { | ||
case "progress", "radius", "outerRingWidth": |
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 could use #keyPath(progress)
, #keyPath(radius)
, etc., here to avoid the string literals.
No description provided.