-
Notifications
You must be signed in to change notification settings - Fork 211
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 convenience FBSnapshotVerifyViewController() function to Swift clients #101
Add convenience FBSnapshotVerifyViewController() function to Swift clients #101
Conversation
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.
You have sold me @natan-anchor. If you can sign the CLA I can merge this PR.
@@ -11,6 +11,14 @@ public extension FBSnapshotTestCase { | |||
func FBSnapshotVerifyView(_ view: UIView, identifier: String? = nil, suffixes: NSOrderedSet = FBSnapshotTestCaseDefaultSuffixes(), perPixelTolerance: CGFloat = 0, overallTolerance: CGFloat = 0, file: StaticString = #file, line: UInt = #line) { | |||
FBSnapshotVerifyViewOrLayer(view, identifier: identifier, suffixes: suffixes, perPixelTolerance: perPixelTolerance, overallTolerance: overallTolerance, file: file, line: line) | |||
} | |||
|
|||
func FBSnapshotVerifyViewController(_ viewController: UIViewController, identifier: String? = nil, suffixes: NSOrderedSet = FBSnapshotTestCaseDefaultSuffixes(), perPixelTolerance: CGFloat = 0, overallTolerance: CGFloat = 0, file: StaticString = #file, line: UInt = #line) { | |||
viewController.view.bounds = UIScreen.main.bounds |
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.
Do you think it is worth passing in the bounds as a parameter in case someone wants to override 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.
I haven't seen a need for that in practice, but would be happy to add it if you think it may be useful!
Sorry to bug you again @natan-anchor but if you could sign the CLA we can merge this PR and cut a patch release with it. |
No worries! I just signed the CLA. |
I don't suppose you signed it with a different GitHub account or email because for some reason it still thinks you haven't signed it. Maybe you need to run the recheck link? |
Yep, that was it. Fixed! |
It is becoming more common to configure
UIViewController
s with view models and snapshot them.ios-snapshot-test-case
is a great tool for this, but unfortunately it doesn't support for this 'out of the box'. This task is slightly more complicated thanFBSnapshotVerifyView(viewController.view)
since one almost always want to ensure the size of the view controller prior to the snapshot (and since some view controllers do layout adjustments inviewWillAppear()/viewDidAppear()
, it is convenient to invoke those as well).I've seen the pattern in this PR adopted in a few large projects which extensively use
ios-snapshot-test-case
. Since it is common, it would be great to have it built in to the library for everyone. Thoughts?cc @efirestone @NickEntin