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

Use "scale" mode as default. #66

Merged
merged 2 commits into from
Sep 29, 2022
Merged

Conversation

mauritsvanrees
Copy link
Member

This cleans up more confusion between mode and direction. See also plone/plone.namedfile#102.
Previously our definition of the IImageScaleFactory interface had the deprecated direction="thumbnail".
Other parts used mode="contain" by default, which does cropping, where in Plone we are used to simple scaling almost everywhere.

Depending on your point of view, you could call this a bug fix (fixing the default to what Plone expects, to avoid surprises).
Or you could call this a breaking change, because the default changes.
It is beta stage, so I am okay with it either way.

This cleans up more confusion between mode and direction.  See also plone/plone.namedfile#102.
Previously our definition of the `IImageScaleFactory` interface had the deprecated `direction="thumbnail"`.
Other parts used `mode="contain"` by default, which does cropping, where in Plone we are used to simple scaling almost everywhere.

Depending on your point of view, you could call this a bug fix (fixing the default to what Plone expects, to avoid surprises).
Or you could call this a breaking change, because the default changes.
It is beta stage, so I am okay with it either way.
@mister-roboto

This comment was marked as resolved.

@mauritsvanrees
Copy link
Member Author

The gh-actions pass. Now I test these two PRs together:

https://github.com/plone/plone.scale/pull/66
https://github.com/plone/plone.namedfile/pull/130

They are not dependent on each other, but they change similar things, so it is good to test them in combination.

Copy link
Contributor

@MrTango MrTango left a comment

Choose a reason for hiding this comment

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

LGTM, and then change make sense, thx @mauritsvanrees

@mauritsvanrees
Copy link
Member Author

Testing these two again on Jenkins 6.0:

https://github.com/plone/plone.scale/pull/66
https://github.com/plone/plone.namedfile/pull/130

@mauritsvanrees
Copy link
Member Author

That should be:

https://github.com/plone/plone.scale/pull/66
https://github.com/plone/plone.namedfile/pull/131

Trying again.

@mauritsvanrees mauritsvanrees merged commit 79f2f13 into master Sep 29, 2022
@mauritsvanrees mauritsvanrees deleted the maurits-direction-versus-mode branch September 29, 2022 18:50
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.

5 participants