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

Adding Aspect Fit/Fill to Image Resizing Processor #597

Closed
TimOliver opened this issue Feb 19, 2017 · 10 comments
Closed

Adding Aspect Fit/Fill to Image Resizing Processor #597

TimOliver opened this issue Feb 19, 2017 · 10 comments

Comments

@TimOliver
Copy link

Hi @onevcat!

Thanks for this awesome library! It's really impressive! :)

I was playing with the ResizingImageProcessor in my app to resize images preload in a UITableView, and there were 2 resizing modes I was looking to use:

  • Aspect fit: where the image stays at the same aspect ratio, but it scaled down to completely fit inside `targetSize.
  • Aspect fill: where the image is kept at the same ratio, but scaled to fill the bounds of targetSize, with the outlying edges getting cropped.

I'd be happy to volunteer to add these. If you'd like that, would it be worth extending ResizingImageProcessor, or making them separate structs?

Thanks!

@TimOliver TimOliver changed the title Adding Aspect Fit/Fill to Image Processor Adding Aspect Fit/Fill to Image Resizing Processor Feb 19, 2017
@onevcat
Copy link
Owner

onevcat commented Feb 19, 2017

Hi, thanks for considering contributing!

That's great if you could implement it! I think just extending the ResizingImageProcessor to contains additional properties would be a totally fine and better way.

Since it should not be a breaking change, so please add this as an option and keep current implementation for default using. Currently, the input size will be respected without aspect fitting or filling. So I guess just add an enum in the ResizingImageProcessor to indicate the resizing strategy might be a good idea. I suggest an interface like:

struct ResizingImageProcessor {
    enum ContentMode {
        case none // This will be the default one
        case aspectFit
        case aspectFill
    }

    init(targetSize: CGSize, mode: ContentMode = .none) { ... }
}

How do you think about it?

@Drusy
Copy link
Contributor

Drusy commented Feb 20, 2017

Hi @onevcat, @TimOliver,

I just had to implement this feature this morning for my project functional scope.
Would you like me to share and make the PR for this or @TimOliver do you want to implement this one ?

Let me know

@onevcat
Copy link
Owner

onevcat commented Feb 20, 2017

👍 @Drusy It's great to hear that you are interested in it too. I'd like to receive a p-r from you!

However, since @TimOliver was earlier and might be still working on this, let's keep for a while to see Tim's comment now. :)

@Drusy
Copy link
Contributor

Drusy commented Feb 20, 2017

@onevcat of course, keep me updated :)

@TimOliver
Copy link
Author

Hey @Drusy, and @onevcat! Thanks for that!

Oh cool! No, I hadn't started it yet. I'm on a deadline to ship an app this week, so I would have had to have started it next week at the earliest.

If you've already got an implementation, then by all means, don't let me stand in the way! :)

@onevcat
Copy link
Owner

onevcat commented Feb 21, 2017

@TimOliver Great! Thank you.

@Drusy :)

@BrikerMan
Copy link

added #599

@Drusy
Copy link
Contributor

Drusy commented Feb 21, 2017

@onevcat After reading @BrikerMan PR, I actually didn't know the existence of a such method func resize(to size: CGSize, for contentMode: UIViewContentMode) -> Image. If I would, I would probably have done the same.

Here is my PR #600 for this feature using your size proxy (no UIKit)

@onevcat
Copy link
Owner

onevcat commented Feb 21, 2017

@TimOliver @BrikerMan @Drusy Thank you all! #600 was merged and I will prepare a release for it later today.

func resize(to size: CGSize, for contentMode: UIViewContentMode) -> Image is now a UIKit specified helper method and we could merge it to the general resize one with new ContentMode enum, to improve readability. That should be another story and maybe I could find some time later for it.

@onevcat onevcat closed this as completed Feb 21, 2017
@TimOliver
Copy link
Author

Awesome! Great work all! :D

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

No branches or pull requests

4 participants