-
Notifications
You must be signed in to change notification settings - Fork 63
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
Reworked module #23
base: master
Are you sure you want to change the base?
Reworked module #23
Conversation
It looks great. What do you think? |
@EyreFree if we're going to break the API, then I believe that a new protocol should be "minimal" and not include methods that can be achieved with other methods from the same protocol. For instance, I need more time to review the rest. |
I've made it via protocol extensions. So no need to implement them any more. extension EFCount {
public func countFromZeroTo(_ endValue: CGFloat, withDuration duration: TimeInterval) {
countFrom(0, to: endValue, withDuration: duration)
}
public func countFrom(_ startValue: CGFloat, to endValue: CGFloat) {
countFrom(startValue, to: endValue, withDuration: 0)
}
public func countFromCurrentValueTo(_ endValue: CGFloat) {
countFromCurrentValueTo(endValue, withDuration: 0)
}
public func countFromZeroTo(_ endValue: CGFloat) {
countFromZeroTo(endValue, withDuration: 0)
}
} Something like syntactic sugar. But yes...naming could be better. I'm not good at 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.
OK, so here is my review:
-
👍Thank you for the protocol, it's welcomed.
-
👎The removal of
setTextValue
is a regression in the usability of this pod. People could previously just define a format conversion between a CGFloat and a String, but now they have to explicitly set the String to a label.text or button.setTitle, which also requires them to write a reference toself
which they may not do correctly. So I've done a pull to attempt to remedy partially to that issue by keeping support for the existingformatBlock
,attributedFormatBlock
andcompletionBlock
in Adding a partial compatibility layer with previous version Kirow/EFCountingLabel#1. -
I'm not sure that we should use
[unowned self]
, as when one will release a UIView, it may callupdate(value, self)
in the deinit block itself. Maybe it's OK, or maybe we could use[weak self]
to be absolutely safe. I could try to investigate that a bit more. Let's note that even if we're sure it won't crash, there is no performance difference betweenweak
andunowned
here considering the refresh rate is only about 30 frames/secondes. -
👍I won't cry at the loss of
public var format: String
, because there were too much prediction attempt on it done withhasIntConversionSpecifier
. UsingformatBlock
+String(format: format, ...)
should be good for everybody.
and add compatibility layer similar to Kirow#1 but via protocol. In result we should have new minimal interface for |
- updated example
- added frame rate change test to example
As the changes start to be too big in my eyes, I'm cherry picking the early commits in a separate PR. See #26. |
fe43677
to
64d66c4
Compare
86b698e
to
d596b83
Compare
well...pretty much changes. Only core logic left.
core interface:
main counter class
EFCounter
adoptEFCount
to implement counter into any other classes I've added:
So in most cases it will be like:
most protocol methods are implemented in extensions.
Also there were a lot of renaming compared to previous version. So there will be no backward compatibility.
Updated example project.
TODO:
README.md
.podspec
UITimingCurveProvider
(i.e.UICubicTimingParameters
andUISpringTimingParameters
)CAMediaTimingFunction
Please review/discuss or apply changes if acceptable.