Test in light and dark mode #265
Conversation
There was a problem hiding this comment.
Thanks @dchohfi, this looks great!
I add just a few small comments and I would like to know the opinion of @ashfurrow or someone more experienced in one topic before approve, just to be sure.
But great work!
| } | ||
|
|
||
| it("fails to find the snapshots due to the custom folder") { | ||
| expect(view).notTo(haveValidSnapshot(named: "something custom")) | ||
| expect(view).notTo(haveValidSnapshot(named: "something custom")) |
| } | ||
|
|
||
| it("finds the snapshots using a custom images directory") { | ||
| expect(view).to(haveValidSnapshot()) | ||
| expect(view).to(recordSnapshot()) |
| expect(view).to(recordDeviceAgnosticSnapshot()) | ||
| } | ||
|
|
||
| it("find the snapshot using a custom image directory for light and dark mode") { | ||
| expect(view).to(recordSnapshot(userInterfaceStyle: .light)) | ||
| expect(view).to(recordSnapshot(userInterfaceStyle: .dark)) |
There was a problem hiding this comment.
I guess all of those also should be haveValid and not record
There was a problem hiding this comment.
I can't get this test to run properly, we still need to record images for this test, that is why I left it like that.
There was a problem hiding this comment.
Here we should stick to AAA (A.K.A Arrange, Act, and Assert) principles, and as we expect two different outputs , should be two isolated unit tests
| // if an userInterfaceStyle is sent, we need to force usesDrawViewHierarchyInRect | ||
| snapshotController.usesDrawViewHierarchyInRect = userInterfaceStyle == nil ? usesDrawRect : true |
There was a problem hiding this comment.
I would like to hear what more experienced in the library thinks about it.
I'm not sure about the side effects of using usesDrawViewHierarchyInRect or not.
There was a problem hiding this comment.
Could you please elaborate on this solution, I'm trying to understand why usesDrawViewHierarchyInRect should be always true when an userInterfaceStyle is set
|
Hey this looks cool! Apologies for not reviewing sooner, things have been hectic. I probably won't get a chance to review this until later this week. Thanks the PR and for your patience 馃檹 |
|
hey @ashfurrow no problem! we're using internally our own fork for now. let me know when you have time, thank you. |
| @@ -8,7 +8,13 @@ public final class DynamicTypeView: UIView { | |||
| label = UILabel() | |||
| super.init(frame: frame) | |||
|
|
|||
| backgroundColor = .white | |||
| backgroundColor = UIColor { traits -> UIColor in | |||
There was a problem hiding this comment.
You don麓t need the else here
backgroundColor = UIColor { traits -> UIColor in
if traits.userInterfaceStyle == .dark {
return .lightGray
}
return .white
}
| @@ -12,19 +12,31 @@ final class BootstrapCustomFormatTests: QuickSpec { | |||
| beforeEach { | |||
| setNimbleTestFolder("CustomFolder") | |||
| view = UIView(frame: CGRect(origin: .zero, size: CGSize(width: 44, height: 44))) | |||
| view.backgroundColor = .blue | |||
| view.backgroundColor = UIColor { traits -> UIColor in | |||
| expect(view).to(recordDeviceAgnosticSnapshot()) | ||
| } | ||
|
|
||
| it("find the snapshot using a custom image directory for light and dark mode") { | ||
| expect(view).to(recordSnapshot(userInterfaceStyle: .light)) | ||
| expect(view).to(recordSnapshot(userInterfaceStyle: .dark)) |
There was a problem hiding this comment.
Here we should stick to AAA (A.K.A Arrange, Act, and Assert) principles, and as we expect two different outputs , should be two isolated unit tests
| view.backgroundColor = UIColor { traits -> UIColor in | ||
| if traits.userInterfaceStyle == .dark { | ||
| return .brown | ||
| } else { |
There was a problem hiding this comment.
Same about the else statement
| } | ||
|
|
||
| it("has a valid snapshot") { | ||
| expect(view).to(haveValidSnapshot()) | ||
| expect(view).to(haveValidSnapshot(userInterfaceStyle: .light)) |
| view.backgroundColor = UIColor { traits -> UIColor in | ||
| if traits.userInterfaceStyle == .dark { | ||
| return .brown | ||
| } else { |
There was a problem hiding this comment.
Same about the else statement
| } | ||
|
|
||
| it("has a valid snapshot to all sizes") { | ||
| expect(view).to(haveValidDynamicSizeSnapshot(sizes: sizes)) | ||
| // expect(view).to(recordDynamicSizeSnapshot(sizes: sizes)) | ||
| } | ||
|
|
||
| it("has a valid snapshot to all sizes in light and dark mode") { | ||
| expect(view).to(haveValidDynamicSizeSnapshot(sizes: sizes, userInterfaceStyle: .light)) |
| // if an userInterfaceStyle is sent, we need to force usesDrawViewHierarchyInRect | ||
| snapshotController.usesDrawViewHierarchyInRect = userInterfaceStyle == nil ? usesDrawRect : true |
There was a problem hiding this comment.
Could you please elaborate on this solution, I'm trying to understand why usesDrawViewHierarchyInRect should be always true when an userInterfaceStyle is set
Hi 馃憢
It is a pleasure to be able to open a PR in this lib, I've been using this for years and years, thank you for creating and supporting it!
I had to add a support for light/dark mode on a private app and thought about opening a PR, lets see if you think this worth to be merged on the main code.
Hopefully I did everything (almost) correct 馃檹
FYI: my last commit - 3fb008d - tries to add a test that wasn't added to the project, and I don't know why it isn't recording snapshots, I might need some help with it.
Anyway, let me know how can I improve this PR and if it makes sense to be added to the main code.