search

thephpleague / glide

Wonderfully easy on-demand image manipulation library with an HTTP based API.
http://glide.thephpleague.com
MIT License
2.55k stars 198 forks source link

Add "fill-max" resizing option #332

Closed kjellknapen closed 2 years ago

kjellknapen commented 2 years ago

I noticed the issue for #206 had a pull request but it hadn't been merged yet because of naming issues. So I went to check in the Imgix api and made changes accordingly. In the imgix api the fill function is used for resizing and filling an image (so this function shoud actually do what is requested in the issue).

So in my PR I changed this function to the function made by @xor22h in the previous PR. Next to that I added the fillmax function to allow users to also fill and preserve ratio as the fill function used to do. If you rather have them the other way around I can change it since this would be a change affecting everyone using the fill method.

kjellknapen commented 2 years ago

If it's possible when the request is good could you add the hacktoberfest-accepted label?

ADmad commented 2 years ago

If you rather have them the other way around I can change it since this would be a change affecting everyone using the fill method.

Breaking backwards compatibility would be problematic. Please keep the current behaviour of fill the same.

kjellknapen commented 2 years ago

@ADmad You are right, I switched them up so it won't result in a breaking change :)

ADmad commented 2 years ago

Wonder why PHP 8.1 is segfaulting. Also can you update the docs too regarding the new option.

ADmad commented 2 years ago

Please rebase your branch onto upstream master where I have fixed the testsuite failure on PHP 8.1.

kjellknapen commented 2 years ago

Please rebase your branch onto upstream master where I have fixed the testsuite failure on PHP 8.1.

Rebased everything!

ADmad commented 2 years ago

Sorry I didn't notice this earlier but codecov says there's no test coverage for the new runFillMaxResize() method. Please add a test for it.

kjellknapen commented 2 years ago

@ADmad Is it okay now?

ADmad commented 2 years ago

Looks like an invocation count needs to be updated to fix the test failure.

ADmad commented 2 years ago

@kjellknapen This will go faster if you run the test suite locally first to ensure it passes :slightly_smiling_face:.

kjellknapen commented 2 years ago

@ADmad Yeah I know, I'm sorry, thought it was going to be a quick and easy fix! Now it should work. Sorry that everything took so long, haven't done a whole lot of open source!

ADmad commented 2 years ago

@kjellknapen No need to apologize, thank you for seeing this through.