Skip to content
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

Merged
merged 7 commits into from
Jan 18, 2022
Merged

Conversation

alexey1312
Copy link
Contributor

  • Added SnapshotTestingHEIC dependence
  • Added SnapshottingHEIC
  • Added SnapshotTestingStitchHEICTests

Copy link
Owner

@Sherlouk Sherlouk left a 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 😄

Comment on lines 75 to 78
let internalStrategy: Snapshotting<UIViewController, UIImage> = .imageHEIC(
precision: precision,
compressionQuality: compressionQuality
)
Copy link
Owner

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".

Copy link
Contributor Author

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)

@alexey1312 alexey1312 requested a review from Sherlouk January 18, 2022 07:41
Copy link
Owner

@Sherlouk Sherlouk left a 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 😄

@Sherlouk Sherlouk merged commit 8e5e510 into Sherlouk:main Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants