-
Notifications
You must be signed in to change notification settings - Fork 13
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
Transition coordinator #63
Conversation
@@ -21,26 +23,47 @@ class BaseContainerViewController: UIViewController { | |||
private lazy var controllerA = ViewControllerA() | |||
private lazy var controllerB = ViewControllerB() | |||
|
|||
private var interactionController: HorizontalPanGestureInteractiveTransition? | |||
private var animationController = WipeTransitionAnimator(direction: .leftToRight) |
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.
What is a WipeTransition? Did you mean Swipe?
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.
A WipeTransition in this case, is in an interactive transition where you can pan from the screen edge to reveal the destination view controller.
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.
Since we're being explicit about leftToRight
vs rightToLeft
, would there be a way to let the system tell us the direction (I'm wondering how this would work when using a right-to-left language)?
Examples/UtiliKit-iOSExample/Container/ContainerExampleViewController.swift
Outdated
Show resolved
Hide resolved
Examples/UtiliKit-iOSExample/Container/InteractiveTransition.swift
Outdated
Show resolved
Hide resolved
Examples/UtiliKit-iOSExample/Container/InteractiveTransition.swift
Outdated
Show resolved
Hide resolved
Examples/UtiliKit-iOSExample/Container/InteractiveTransition.swift
Outdated
Show resolved
Hide resolved
Examples/UtiliKit-iOSExample/Container/WipeTransitionAnimator.swift
Outdated
Show resolved
Hide resolved
configureInitialState(for: destination, with: transitionContext) | ||
let timingParameters = UICubicTimingParameters(animationCurve: .easeInOut) | ||
let propertyAnimator = UIViewPropertyAnimator(duration: transitionDuration(using: transitionContext), timingParameters: timingParameters) | ||
propertyAnimator.addAnimations { [unowned self] in |
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.
what is people's opinion of using unowned self
in the open source projects?
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 have the same opinion as I do in client projects - if you can reason that the self being referenced as unowned is the object retaining the closure doing the referencing then you are safe to use it as you can guarantee it won't be deallocated when that closure executes.
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 agree with Will on this one. I'm fine with 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.
Personally, I'm not a fan of unowned
. As a reader, I either have to put all of my trust in the author that they knew what they're doing or spend extra time to verify they did it correctly.
In this case, I'm not even sure if I'm able to quickly understand that this is a "safe" use of unowned
. The closure is owned by the propertyAnimator
, which ends up being returned from this function so someone else (whoever interacts with WipeTransitionAnimator
through the UIViewControllerAnimatedTransitioning
protocol) must be retaining it in a way that's safe.
I get that unowned
might be convenient sometimes when you need to access properties or functions on self
from inside the closure, but in this example, it doesn't matter. Instead, it feels like we are forced to deal with all the extra cognitive load (or potentially a bug/crash) just to avoid doing some optional chaining.
propertyAnimator.addAnimations { [weak self] in
self?.configureFinalState(forSource: source, destination: destination, context: transitionContext)
}
Since this is "example" code, I suppose it's okay to leave it as unowned
, but even from a best-practice standpoint I'd make the argument that encouraging weak
would be better.
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 property animator is also retained by the UIViewControllerAnimatedTransitioning
object creating it.
I will make these weak
.
source?.didMove(toParentViewController: nil) | ||
destination.didMove(toParentViewController: self) | ||
#endif | ||
func finishTransitioning(from source: UIViewController?, to destination: UIViewController, success: Bool, animated: Bool) { |
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.
Can you clean up the duplicate code contained in this function?
destination.endAppearanceTransition()
is written 4 times for example
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.
Done. Also, now that we're supporting Swift 5 we should be ok to remove the pre- Swift 4.2 code.
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.
Great
Sources/UtiliKit/Container/Transitioning/ContainerViewControllerTransitionCoordinator.swift
Outdated
Show resolved
Hide resolved
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.
Looks good to me! This is really going to make ContainerViewController a great base for many custom UI situations.
source?.removeFromParent() | ||
|
||
visibleController = destination | ||
|
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.
Extra line here can be removed.
@ganttastic Agreed! I'm hoping that in the near future (possibly if we split this off into its own library), we provide several examples on how to use it. Everything from the most basic parent/child relationship to complex containers that animate between their children. |
OK, changelog entry added and fixed some of the duplicate code issues and grammar mistakes @earlgaspard found. Let me know if there's anything else. |
guard let nextChild = self.containerViewController.child(following: currentController) else { return } | ||
self.animationController.transitionDirection = .rightToLeft | ||
self.containerViewController.transitionToController(for: nextChild) | ||
self.configureInteractiveTransitionToFollowingChild(of: currentController) |
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.
Extremely minor, but these might read better something like:
self.configureInteractiveTransitionToChild(preceding: currentController)
self.configureInteractiveTransitionToChild(following: currentController)
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 agree - fixed.
Going to give @earlgaspard and @ganttastic a chance to take another look after the last round of tweaks, otherwise I'll look to merge in later today / tomorrow. |
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 good with the changes.
There's a lot going on here. Basically it allows you to use
UIViewControllerInteractiveTransitioning
andUIViewControllerTransitionCoordinator
objects with the container.I'm also thinking we might want to split this into it's own Pod as it's getting so large.
I'm still making tweaks to the protocols, but I wanted to get other eyes on it for more improvements.