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
Add a ContainerViewController subspec #2
Conversation
@BottleRocketStudios/ios-developers Please take a look at this when you get a chance. |
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. ContainerViewController looks nice!
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 cool! Just a few minor fixes needed I think.
destination = destinationViewController | ||
completion = completionHandler | ||
|
||
viewControllers = [UITransitionContextViewControllerKey.from : sourceViewController, |
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 you can use type inference here, right?
viewControllers = [.from : sourceViewController,
.to : destinationViewController]
} | ||
|
||
var targetTransform: CGAffineTransform { | ||
return CGAffineTransform.identity |
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 use type inference here as well.
return .identity
|
||
//MARK: Properties | ||
public var children: [Child] = [] | ||
public private(set) var isTransitioning: Bool = 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.
public
private
?
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.
Public getter, private setter. Not sure exactly how valuable it is to an outside component - but assumed it couldn't hurt.
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.
Ah, I forgot this is a framework so it just looked like a typo at first.
transition(to: child.viewController) | ||
} | ||
|
||
open func transitionToController(withTitle title: String) { |
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 this an API we want as part of this library? Since titles are susceptible to changing from UX, it seems like we wouldn't be doing anyone a service if we gave them the option to use this in production code. We may want to declare this part of the extension in the Example code instead.
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.
Fair point. Will change.
} | ||
|
||
func finishTransitioning(from source: UIViewController?, to destination: UIViewController, animated: Bool) { | ||
source.map { removeSourceControllerFromContainer($0, animated: animated) } |
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.
Nice uses of map
and flatMap
in here!
@tylermilner Requested changes have been made |
Added a container view controller. Doesn't present any visuals for navigation of it's own, just the child view controllers. Can probably write some unit tests for parts of the functionality.