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

Pass crop parameter in default EssencePicture#picture_url_options #2098

Merged
merged 1 commit into from
May 13, 2021

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented May 13, 2021

What is this pull request for?

Prior to this commit, calling my_essence.picture_url without
parameters would return an uncropped image be default. In order to get
the cropped picture, one would have to add my_essence.picture_url(crop: true). This is unintuitive: For an EssencePicture that I have cropped
as a user (crop values are present), essence.picture_url as well as
essence.ingredient should return the cropped version of the image.

Notable changes (remove if none)

EssencePicture#ingredient and EssencePicture.picture_url now return a cropped image by default if cropping is enabled in the element settings or if crop values are present.

Checklist

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change

Prior to this commit, calling `my_essence.picture_url` without
parameters would return an uncropped image be default. In order to get
the cropped picture, one would have to add `my_essence.picture_url(crop:
true)`. This is unintuitive: For an EssencePicture that I have cropped
as a user (crop values are present), `essence.picture_url` as well as
`essence.ingredient` should return the cropped version of the image.
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

I agree.

BUT, the impact his has is:

If the essence picture has crop values, but the settings[:crop] has since been set to false it still crops although it has been disabled after the essence got created. This makes the crop setting less important and removes it as a way to disable cropping later. A very rare use case IMO, but still worth noting and therefore a breaking change for v6

Thanks anyhow. I think this is a good change.

@tvdeyen tvdeyen added this to the 6.0 milestone May 13, 2021
@mamhoff mamhoff merged commit ce23e3b into AlchemyCMS:main May 13, 2021
@mamhoff mamhoff deleted the fix-cropped-essence-picture-url branch May 13, 2021 13:12
robinboening pushed a commit to robinboening/alchemy_cms that referenced this pull request Jul 2, 2021
…chemyCMS#2098)

Prior to this commit, calling `my_essence.picture_url` without
parameters would return an uncropped image be default. In order to get
the cropped picture, one would have to add `my_essence.picture_url(crop:
true)`. This is unintuitive: For an EssencePicture that I have cropped
as a user (crop values are present), `essence.picture_url` as well as
`essence.ingredient` should return the cropped version of the image.
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.

2 participants