Skip to content

Use the scale factor of the device's main screen when scaling the image. #515

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 15, 2018
Merged

Use the scale factor of the device's main screen when scaling the image. #515

merged 2 commits into from
Jul 15, 2018

Conversation

VincentSit
Copy link
Member

This PR solves the image scaling issue I commented on #446.

Checklist

  • I checked the Contributing Guidelines before creating this request.
  • New extensions are written in Swift 4.
  • New extensions support iOS 8.0+ / tvOS 9.0+ / macOS 10.10+ / watchOS 2.0+.
  • I have added tests for new extensions, and they passed.
  • All extensions have a clear comments explaining their functionality, all parameters and return type in English.
  • All extensions are declared as public.
  • I have added a changelog entry describing my changes.

@VincentSit VincentSit changed the title Use the scale factor of the device's main screen when scaling. Use the scale factor of the device's main screen when scaling the image. Jul 13, 2018
@SwifterSwiftBot
Copy link

SwifterSwiftBot commented Jul 13, 2018 •
edited
Loading

1 Warning
⚠️ Consider adding tests for new extensions or updating existing tests for a modified SwifterSwift extension
4 Messages
📖 Thank you for submitting a pull request to SwifterSwift. The team will review your submission as soon as possible.
📖 Executed 487 tests, with 0 failures (0 unexpected) in 2.906 (3.481) seconds
📖 Executed 457 tests, with 0 failures (0 unexpected) in 1.913 (2.412) seconds
📖 Executed 331 tests, with 0 failures (0 unexpected) in 1.277 (1.508) seconds

Generated by 🚫 Danger

@codecov
Copy link

codecov bot commented Jul 13, 2018 •
edited
Loading

Codecov Report

Merging #515 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #515   +/-   ##
=======================================
  Coverage   93.81%   93.81%           
=======================================
  Files          66       66           
  Lines        2765     2765           
=======================================
  Hits         2594     2594           
  Misses        171      171
Flag Coverage Δ
#ios 93.81% <100%> (ø) ⬆️
#osx 54.71% <0%> (ø) ⬆️
#tvos 93.81% <100%> (ø) ⬆️
Impacted Files Coverage Δ
Sources/Extensions/UIKit/UIImageExtensions.swift 97.82% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdef062...6fe93eb. Read the comment docs.

LucianoPAlmeida
LucianoPAlmeida previously approved these changes Jul 14, 2018
View reviewed changes
Copy link
Member

@LucianoPAlmeida LucianoPAlmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@SD10 SD10 requested a review from guykogus July 14, 2018 04:12
guykogus
guykogus requested changes Jul 14, 2018
View reviewed changes
Sources/Extensions/UIKit/UIImageExtensions.swift Outdated
@@ -74,7 +74,7 @@ public extension UIImage {
public func scaled(toHeight: CGFloat, opaque: Bool = false, with orientation: UIImageOrientation? = nil) -> UIImage? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're at it fixing these functions, can you remove the orientation param for both of them? It's unused.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

LucianoPAlmeida
LucianoPAlmeida approved these changes Jul 15, 2018
View reviewed changes
Copy link
Member

@LucianoPAlmeida LucianoPAlmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@VincentSit Thank you for this fix 👍

@LucianoPAlmeida LucianoPAlmeida merged commit 70bd4a6 into SwifterSwift:master Jul 15, 2018
@SD10
Copy link
Member

SD10 commented Jul 15, 2018

Thank you for contributing to SwifterSwift! I've invited you to join the SwifterSwift GitHub organization - no pressure to accept! If you'd like more information on what that means, check out our contributing guidelines. Feel free to reach out if you have any questions! 😃

@VincentSit
Copy link
Member Author

Thank you all.

@shannonchou shannonchou mentioned this pull request Aug 14, 2019
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guykogus guykogus guykogus approved these changes

@LucianoPAlmeida LucianoPAlmeida LucianoPAlmeida approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@VincentSit @SwifterSwiftBot @SD10 @guykogus @LucianoPAlmeida