Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

katsuroo
Copy link
Contributor

@katsuroo katsuroo commented Jan 8, 2019

No description provided.

@katsuroo katsuroo requested a review from sharplet January 8, 2019 15:27
Copy link
Contributor

@sharplet sharplet left a 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;
Copy link
Contributor

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)
Copy link
Contributor

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(),
Copy link
Contributor

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)
Copy link
Contributor

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)
Copy link
Contributor

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),
Copy link
Contributor

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
Copy link
Contributor

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),
Copy link
Contributor

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,
Copy link
Contributor

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":
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants