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

Fix #769 #770

Closed
wants to merge 6 commits into from
Closed

Fix #769 #770

wants to merge 6 commits into from

Conversation

deviprsd
Copy link
Contributor

This permanently and efficiently fixes the problem

$ratio = max($widthRatio, $heightRatio);
} else {
$ratio = min($widthRatio, $heightRatio);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

could this be $ratio = ($width === null || $height === null) ? max($widthRatio, $heightRatio) : min($widthRatio, $heightRatio); ?

Copy link
Contributor Author

@deviprsd deviprsd Aug 15, 2016

Choose a reason for hiding this comment

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

Of course, either way that is just a style thing, I don't think it will be a performance issue, so I'm okay with anything. you might see this info http://stackoverflow.com/a/11643364

Copy link
Contributor

Choose a reason for hiding this comment

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

in Symfony we generally use null === $foo

If image smaller in dimensions than the max arguments, Downscale doesn't
upscale, and if image greater in dimensions than min arguments Upscale
doesn't downscale.

if ($ratio > 1) {
return $image;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be controversial thing, discuss on this 🎱

Copy link
Collaborator

@alexwilson alexwilson Aug 15, 2016

Choose a reason for hiding this comment

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

hah, now that's my kinda fix! 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

this should be an option, defaulting to off

Copy link
Contributor Author

@deviprsd deviprsd Aug 16, 2016

Choose a reason for hiding this comment

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

I was thinking more like, creating another filter "scale" which will do either, the filters should do only what they should, I don't know maybe it's just clear that way, moreover downscale and upscale should be derived from scale

Copy link
Contributor

Choose a reason for hiding this comment

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

fine for me too

@alexwilson
Copy link
Collaborator

The diff for this PR is a little large, would it make sense to separate the fixes for #728 into a separate PR?

Otherwise, this makes a lot of sense - Thoughts @lsmith77 ?

@deviprsd
Copy link
Contributor Author

@antoligy Wouldn't be such a bad idea! Just waiting for @lsmith77 to check it.

@deviprsd
Copy link
Contributor Author

Then I'll close this and work on the filter, with another feature for these filters

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.

3 participants