-
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
Add postTransitionBehavior to ContainerViewController #88
Conversation
CHANGELOG.md
Outdated
@@ -6,6 +6,10 @@ | |||
[Will McGinty](https://github.com/willmcginty) | |||
[#87](https://github.com/BottleRocketStudios/iOS-UtiliKit/pull/82) | |||
|
|||
* Add `postTransitionBehavior` to `ContainerViewController`, automating some common child management scenarios | |||
[Dimitar Milinski](https://github.com/dmilinski) | |||
[#88](https://github.com/BottleRocketStudios/iOS-UtiliKit/pull/83) |
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 numbers should match.
[#88](https://github.com/BottleRocketStudios/iOS-UtiliKit/pull/83) | |
[#88](https://github.com/BottleRocketStudios/iOS-UtiliKit/pull/88) |
CHANGELOG.md
Outdated
@@ -6,6 +6,10 @@ | |||
[Will McGinty](https://github.com/willmcginty) | |||
[#87](https://github.com/BottleRocketStudios/iOS-UtiliKit/pull/82) |
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 correct this too.
[#87](https://github.com/BottleRocketStudios/iOS-UtiliKit/pull/82) | |
[#87](https://github.com/BottleRocketStudios/iOS-UtiliKit/pull/87) |
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.
Oops, good catch
removeAllNonVisibleChildren(except: []) | ||
case .removeAllNonVisibleChildrenExcept(let identifiers): | ||
removeAllNonVisibleChildren(except: identifiers) | ||
case .none: break |
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 don't think UtiliKit is using SwiftLint but I know you could get the following using it:
Switch Case on Newline Violation: Cases inside a switch should always be on a newline (switch_case_on_newline)
Can you move the break to a new line.
case .none: break | |
case .none: | |
break |
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
@@ -9,6 +9,12 @@ import UIKit | |||
|
|||
open class ContainerViewController: UIViewController { | |||
|
|||
// MARK: Subtypes | |||
public enum PostTransitionBehavior { | |||
case removeAllNonVisibleChildren |
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.
Could .removeAllNonVisibleChildren
be a static let that calls .removeAllNonVisibleChildrenExcept([])
so we don't have to handle it in switches and stuff?
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, good call
// MARK: Subtypes | ||
public enum PostTransitionBehavior { | ||
case removeAllNonVisibleChildren | ||
case removeAllNonVisibleChildrenExcept([AnyHashable]) |
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.
Could also have a default
or none
case here and then make the property non-optional?
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. I was on the fence about this when I originally worked on it. It actually ends up identical when we handle it in a switch, since optionals have a .none
case anyway lol
Codecov Report
@@ Coverage Diff @@
## master #88 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 5
Lines ? 474
Branches ? 0
=======================================
Hits ? 474
Misses ? 0
Partials ? 0 Continue to review full report at Codecov.
|
No description provided.