Remove self where not necessary #446
Conversation
Generated by 🚫 Danger |
SD10
changed the title
IBAnimatable/Animatable.swift
Outdated
| @@ -579,7 +579,7 @@ fileprivate extension Animatable where Self: UIView { | |||
| } | |||
|
|
|||
| var screenSize: CGSize { | |||
| return self.window?.screen.bounds.size ?? CGSize.zero | |||
| return window?.screen.bounds.size ?? CGSize.zero | |||
There was a problem hiding this comment.
I think we can remove CGSize and just use .zero.
| @@ -21,14 +21,13 @@ public class FlipAnimator: NSObject, AnimatedTransitioning { | |||
| fileprivate var horizontal: Bool = false | |||
|
|
|||
| // MARK: - Life cycle | |||
| public init(from direction: TransitionAnimationType.Direction, transitionDuration: Duration) { | |||
| public init(from direction: TransitionAnimationType.Direction, duration: Duration) { | |||
There was a problem hiding this comment.
That will introduce breaking changes. Please update the CHANGELOG.md
There was a problem hiding this comment.
Do I just need to reference this Pull Request or should I actually state the parameter name change? I updated the CHANGELOG but let me know if I should change it. I'm learning what an API breaking change is now 😃
There was a problem hiding this comment.
I don't think you should add an entry for the change of self. since it's only internal changes, but you should add an entry in the API breaking change to explain the prototype change transitionDuration -> duration because that may break for the users.
There was a problem hiding this comment.
Ok I did this plus changed the remaining 3 that were not changed as to keep the API consistent. Thank you for clarifying @tbaranes
|
@SD10 thanks for fixing that in no time ⚡️⚡️⚡️, you rock 🚀. And please have a look at the comment on CHANGELOG.md. |
JakeLin
changed the title
SD10
force-pushed
the
remove-superfluous-self
branch
from
999a543
to
5046753
Compare
CHANGELOG.md
Outdated
| @@ -11,6 +11,8 @@ All notable changes to this project will be documented in this file. | |||
| - Replace all `SystemAnimator` classes with `SystemTransitionAnimator` | |||
| [#427](https://github.com/IBAnimatable/IBAnimatable/pull/427) by [@SD10](https://github.com/sd10) | |||
| - `PresentationDesignable` now supports `contextFrameForPresentation` which allow you to present a controller with a custom configuration over another instead of being in fullscreen. Imitates `UIModalPresentationStyle.currentContext` | |||
| - Remove the use of `self` where not necessary | |||
There was a problem hiding this comment.
@SD10 sorry, I don't think we need to put the title as "Remove the use of self where not necessary" for breaking changes, because it doesn't make sense for the user. I meant we should specify the API changes. something like Change the initializer for PortalAnimator, NatGeoAnimator, TurnAnimator, CardsAnimator, FlipAnimator, SlideAnimator, ZoomAnimator and DropDownAnimator. Rename the parameter from transitionDuration to duration.
And please add by [@SD10](https://github.com/sd10) as well.
SD10
force-pushed
the
remove-superfluous-self
branch
from
5046753
to
83da9bb
Compare
CHANGELOG.md
Outdated
| @@ -11,6 +11,8 @@ All notable changes to this project will be documented in this file. | |||
| - Replace all `SystemAnimator` classes with `SystemTransitionAnimator` | |||
| [#427](https://github.com/IBAnimatable/IBAnimatable/pull/427) by [@SD10](https://github.com/sd10) | |||
| - `PresentationDesignable` now supports `contextFrameForPresentation` which allow you to present a controller with a custom configuration over another instead of being in fullscreen. Imitates `UIModalPresentationStyle.currentContext` | |||
| - transition Animator classes initializer parameter changed from transitionDuration -> duration | |||
There was a problem hiding this comment.
One last point: can you uppercase the first character and credit yourself?
SD10
force-pushed
the
remove-superfluous-self
branch
from
83da9bb
to
fca5b50
Compare
Summary of Pull Request:
Remove use of
selfwhere not necessary, #445