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 support for view controller presentation #198
Conversation
SwiftMessages/Animator.swift
Outdated
|
||
var showDuration: TimeInterval? { get } | ||
|
||
var hideDuration: TimeInterval? { get } |
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.
Why Optional? Why not a default value of 0? What does nil
mean in this case?
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.
nil
would mean that the duration is unknown, such as if the animation was being provided by UIDynamicAnimator
.
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 it could be helpful to explain that in a comment, since it's a public API and not immediately obvious (at least to me).
SwiftMessages/BaseView.swift
Outdated
/* | ||
MARK: - Drop shadow | ||
*/ | ||
|
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.
This MARK
has no code below 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.
Will remove
SwiftMessages/StoryboardSegue.swift
Outdated
import UIKit | ||
|
||
/** | ||
A configurable `UIStoryboardSegue subclass that utilizes SwiftMessages to |
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.
Missing a backtick after UIStoryboardSegue
?
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.
Got it
SwiftMessages/StoryboardSegue.swift
Outdated
public var containment: Containment = .content | ||
|
||
/** | ||
Specifies how much the view controller's view is inset from it's superview |
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.
its
not it's
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.
👍
SwiftMessages/StoryboardSegue.swift
Outdated
set { messenger.defaultConfig.presentationStyle = newValue } | ||
} | ||
|
||
// The dim mode to use. See the SwiftMessages.DimMode documentation for details. |
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 comment here and on presentationStyle
should be ///
?
SwiftMessages/StoryboardSegue.swift
Outdated
extension StoryboardSegue { | ||
private class Shower: NSObject, UIViewControllerAnimatedTransitioning { | ||
|
||
fileprivate private(set) var completeTransition: ((_: Bool) -> Void)? |
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 the _:
in completeTransition: ((_: Bool) -> Void)?
needed? Can it just be completeTransition: ((Bool) -> Void)?
?
…bile/SwiftMessages into work/viewControllers # Conflicts: # SwiftMessages/SwiftMessagesSegue.swift
Had to do this to work with Carthage due to a bug Carthage/Carthage#2549
…bile/SwiftMessages into work/viewControllers
…bile/SwiftMessages into work/viewControllers
…bile/SwiftMessages into work/viewControllers
No merge please. Just comments.