Merged
Conversation
…nd of the URL instead of the front
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Baroshem
reviewed
Feb 9, 2024
Baroshem
left a comment
There was a problem hiding this comment.
Few questions from my side.
Also, do you expect this change to cause some breaking changes?
…m previous implementation
Merged
colbyfayock
added a commit
that referenced
this pull request
Feb 15, 2024
This reworks cropping and resizing to support a bigger effort to
correctly apply these transformations at the right point within the
transformation string.
The current mechanism applies the core width and height to the front of
the transformation string, with the idea of providing a "canvas" for
someone to work from, but this is unintuitive, where a width and height
would be the expected final resize, which may not be the case with other
transformations (particularly dynamically generated responsive images).
This removes the widthResize property which applied the transformation
the end, where the core width and height resizing now takes place.
To fill in for use cases where it made sense to apply a "base" cropping
and resizing effect, a new `base*` property was added for width, height,
crop, gravity, and zoom, to accommodate the situation where one might
want to resize, apply transformations, then obtain the true width and
height with a final resize as part of a 2-stage crop and resize.
In the example where
**Somewhat of a tl;dr**
This PR primarily reverses the order in which the parameters are
applied, opting for the primary width and height to be the final
transformation applied to avoid attempting to more accurately describe
what the user's intent is when passing in a width and height. As such,
the name of the options for the 1st stage is now `base*`
**Examples**
Previously when using an effect with a width:
```
constructCloudinaryUrl({
options: {
width: 100,
grayscale: true
}
})
```
The resulting transformation would yield: `c_limit,w_960/e_grayscale`
However, it would now yield: `e_grayscale/c_limit,w_960`
This is more relevant when using the 2-stage resizing, where previously:
```
constructCloudinaryUrl({
options: {
width: 100,
widthResize: 200
}
})
```
The resulting transformation would yield: `c_limit,w_100/c_limit,w_200`
Where now:
```
constructCloudinaryUrl({
options: {
width: 100,
baseResize: 200
}
})
```
The resulting transformation would yield: `c_limit,w_200/c_limit,w_100`
**Use Case**
A big use case around why this is important is limiting the size of an
image prematurely.
In an example such as:
```
constructCloudinaryUrl({
options: {
width: 100,
widthResize: 200
}
})
```
The resulting transformation would yield: `c_limit,w_100/c_limit,w_200`
This is problematic if the original image is greater than or equal to
200, as with the first transformation applied, the image will not be
able to be sized organically above 100. If using a crop that allows
upscaling, it would not result in using the original image, meaning it
would scale to 200 with reduced quality.
The effect is largely the same in what it achieves, but the use case and
intent should be more accurate with this PR.
Relevant:
cloudinary-community/next-cloudinary#327
BREAKING CHANGE: Removes widthResize in favor of width, baseWidth added to replace original width application
github-actions bot
pushed a commit
that referenced
this pull request
Feb 15, 2024
# [@cloudinary-util/url-loader-v5.0.0-beta.1](https://github.com/colbyfayock/cloudinary-util/compare/@cloudinary-util/url-loader-v4.2.0...@cloudinary-util/url-loader-v5.0.0-beta.1) (2024-02-15) * Reworking Resizing / Cropping (#139) ([89bf6ab](89bf6ab)), closes [#139](#139) ### BREAKING CHANGES * Removes widthResize in favor of width, baseWidth added to replace original width application
colbyfayock
added a commit
that referenced
this pull request
Feb 26, 2024
# Description * Reworks cropping and resizing to support a bigger effort to correctly apply these transformations at the right point within the transformation string. #139 BREAKING CHANGE: Removes widthResize in favor of advanced crop prop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This reworks cropping and resizing to support a bigger effort to correctly apply these transformations at the right point within the transformation string.
The current mechanism applies the core width and height to the front of the transformation string, with the idea of providing a "canvas" for someone to work from, but this is unintuitive, where a width and height would be the expected final resize, which may not be the case with other transformations (particularly dynamically generated responsive images).
This removes the widthResize property which applied the transformation the end, where the core width and height resizing now takes place.
To fill in for use cases where it made sense to apply a "base" cropping and resizing effect, a new
base*property was added for width, height, crop, gravity, and zoom, to accommodate the situation where one might want to resize, apply transformations, then obtain the true width and height with a final resize as part of a 2-stage crop and resize.In the example where
Somewhat of a tl;dr
This PR primarily reverses the order in which the parameters are applied, opting for the primary width and height to be the final transformation applied to avoid attempting to more accurately describe what the user's intent is when passing in a width and height. As such, the name of the options for the 1st stage is now
base*Examples
Previously when using an effect with a width:
The resulting transformation would yield:
c_limit,w_960/e_grayscaleHowever, it would now yield:
e_grayscale/c_limit,w_960This is more relevant when using the 2-stage resizing, where previously:
The resulting transformation would yield:
c_limit,w_100/c_limit,w_200Where now:
The resulting transformation would yield:
c_limit,w_200/c_limit,w_100Use Case
A big use case around why this is important is limiting the size of an image prematurely.
In an example such as:
The resulting transformation would yield:
c_limit,w_100/c_limit,w_200This is problematic if the original image is greater than or equal to 200, as with the first transformation applied, the image will not be able to be sized organically above 100. If using a crop that allows upscaling, it would not result in using the original image, meaning it would scale to 200 with reduced quality.
The effect is largely the same in what it achieves, but the use case and intent should be more accurate with this PR.
Relevant: cloudinary-community/next-cloudinary#327