-
Notifications
You must be signed in to change notification settings - Fork 12
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 HEIC format #6
Conversation
alexey1312
commented
Jan 17, 2022
- Added SnapshotTestingHEIC dependence
- Added SnapshottingHEIC
- Added SnapshotTestingStitchHEICTests
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.
Thanks for reaching out and raising this PR! I think this is a great addition and should be encouraged to users.
Left a comment to slightly rework the implementation as to reduce the amount of duplication - let me know if you want to discuss this some more though.
I appreciate the contribution! Thank you 😄
let internalStrategy: Snapshotting<UIViewController, UIImage> = .imageHEIC( | ||
precision: precision, | ||
compressionQuality: compressionQuality | ||
) |
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'm a little wary that we have so much duplication in this class for what is essentially just these four lines.
Could I maybe propose a slight change to the public API and thus would lead to a reduction in duplication?
.stitch(
strategies: [
("iPhone 8", .image(on: .iPhone8)),
("iPhone 8 Plus", .image(on: .iPhone8Plus)),
],
format: .heic(compression: .lossless) // new
)
We simply add (to the existing functions) a new format parameter. This would be a new enum which has the values png
and heic
. It would default to PNG for backwards compatibility.
Inside the stitch function we would switch on that format value in order to choose the "internal strategy".
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 agree, it was a very quick decision. Updated PR)
Formatting Add format property
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.
Tested locally and all passing. Backwards compatibility maintained.
Awesome work! Thank you for adding this 😄