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

EZP-31258: Use "inset" for "scaledown" aliases to avoid undesired image cropping #2907

Merged
merged 1 commit into from
Jan 20, 2020

Conversation

peterkeung
Copy link
Contributor

@peterkeung peterkeung commented Dec 28, 2019

Question Answer
JIRA issue EZP-31258
Bug/Improvement yes
New feature no
Target version 6.13
BC breaks yes
Tests pass yes
Doc needed no

All "scaledown" aliases (https://github.com/ezsystems/ezpublish-kernel/blob/master/eZ/Bundle/EzPublishCoreBundle/Resources/config/image.yml#L174) are using the "outbound" thumbnail mode. This is because the Liip thumbnail filter loader is expecting the text "inset" (https://github.com/liip/LiipImagineBundle/blob/master/Imagine/Filter/Loader/ThumbnailFilterLoader.php#L25) instead of "ImageInterface::THUMBNAIL_INSET" (https://github.com/ezsystems/ezpublish-kernel/blob/7.5/eZ/Bundle/EzPublishCoreBundle/Imagine/Filter/Loader/ScaleWidthDownOnlyFilterLoader.php#L30). As a result, the image aliases are subject to undesirable cropping.

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

+1, but please rebase this on 6.13 branch, from my perspective this is a bug (wrong use of Liip Imagine API).

@peterkeung peterkeung force-pushed the EZP-31258-image_alias_inset branch from 9243e36 to dedc6ec Compare December 30, 2019 16:10
@peterkeung
Copy link
Contributor Author

Rebase on 6.13 branch complete

Copy link
Member

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

@peterkeung please also change the target branch to 6.13.

@peterkeung
Copy link
Contributor Author

Target changed

@andrerom andrerom changed the base branch from master to 6.13 January 2, 2020 08:46
@adamwojs
Copy link
Member

adamwojs commented Jan 7, 2020

I agree with the PR changes, IMHO image cropping is undesired in "scaledown" aliases. I'm only afraid that some projects might rely on current behavior (dimensions of result image), so we should document upgrade steps. Small comparison:

Before After
image image

@adamwojs adamwojs requested a review from alongosz January 7, 2020 11:45
@alongosz
Copy link
Member

alongosz commented Jan 7, 2020

I'm only afraid that some projects might rely on current behavior

@adamwojs In this particular case, given the example you've shown I cannot imagine someone expecting this buggy behavior ;)

@adamwojs
Copy link
Member

adamwojs commented Jan 7, 2020

@adamwojs In this particular case, given the example you've shown I cannot imagine someone expecting this buggy behavior ;)

I mean the dimensions of the resulting image, not cropping (sorry for not being precise) 😛

@micszo micszo self-assigned this Jan 16, 2020
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

QA Approved on eZ Platform EE v2.5.8 with diff.
QA Approved on eZ Studio v1.13.5 with branch.

@micszo micszo removed their assignment Jan 20, 2020
@lserwatka lserwatka merged commit 24c3443 into ezsystems:6.13 Jan 20, 2020
@lserwatka
Copy link
Member

@alongosz could you merge this up?

@alongosz
Copy link
Member

Merged up to 7.5 and master.

Thank you @peterkeung 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants