search

FriendsOfFlarum / upload

The file upload extension with insane intelligence for your Flarum forum.
https://discuss.flarum.org/d/4154
MIT License
177 stars 96 forks source link

Apply height limit when resizing and image #219

Closed fran-f closed 4 years ago

fran-f commented 4 years ago

This pull request changes the resize operation to apply to both dimensions.

The first commit updates the resize() method as discussed in #218. The second renames the setting from resizeMaxWidth to resizeMaxSize.

clarkwinkelmann commented 4 years ago

I just replied to the issue without realizing you had actually opened the PR already. Thanks!

Does resize() with two parameters and aspectRatio() actually resizes according to the largest dimension? I'm surprised this case is not explained in Intervention's own documentation.

I'm not sure what to think of the new setting name. Size could mean a lot of things, I feel like we often use it regarding file size in MB. I'd almost prefer to keep Width here. Or maybe resizeMaxWidthOrHeight. Or is there a better name option?

fran-f commented 4 years ago

I wrote a small test to double check, and yes, it works as expected: the image is resized so that the largest dimension no greater than the given limit; the other dimension is scale to maintain the aspect ratio.

Size is a bit ambiguous, yes. I could change it to resizeMaxDimension, or drop the second commit and leave it as it is 🙂

fran-f commented 4 years ago

Size is a bit ambiguous, yes. I could change it to resizeMaxDimension, or drop the second commit and leave it as it is 🙂

Any preferences? @clarkwinkelmann

clarkwinkelmann commented 4 years ago

I'd say keep the original name from before the PR.

I'm hoping we get a second opinion from another FriendsOfFlarum member on this PR before merging.

clarkwinkelmann commented 4 years ago

I cherry-picked the first commit into fccf1b475ceec18ac302637adb8235bf8b145cb1

fran-f commented 4 years ago

Thank you!