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

Ensure original size is included if any widths are larger and !allowUpscale #190

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

JaredReisinger
Copy link

@JaredReisinger JaredReisinger commented Aug 6, 2023

Change Image.getValidWidths() logic to ensure original width is included if any larger-than-original widths are requested in the !allowUpscale case.

Added test to cover this scenario.

Fixes #184

Note that the implementation here is slightly different from the one mentioned in #184; I didn't like the overly-complicated map() logic with it all crammed in the first line, and it's important that the string->int parsing in line 186 happens before we attempt to compare widths. Plus, adding an additional map() call—but only when !allowUpscale—is balanced by the removal of the now-not-needed filter(), and we also no longer need to special-case the "we had valid widths, but filtered removed them all, so we need to push the original size back into the list" scenario.

From a "what do you expect" standpoint, there are cases where you ask for a specific list of widths (like [300, 1200]), and you might get a width not on that list ([300, 840]). This already happens, though, when upscaling is disabled and you ask only for widths larger than the original. This effectively enables the list of widths to include both a thumbnail width and a maximum width at the same time, ensuring that images narrower than the maximum don't result in only the thumbnail size. (This is useful in photo gallery/lightbox scenarios.)

…pscale

Change `Image.getValidWidths()` logic to ensure original width is included
if larger-than-original widths are requested in the !allowUpscale case.

Added test to cover this scenario.

Fixes 11ty#184
@zachleat
Copy link
Member

zachleat commented Feb 2, 2024

I will merge this but I think I’m going to add an additional threshold so that we don’t get [300, 310] width outputs. It needs to be bigger than the previous size by a certain threshold to be added.

@zachleat zachleat added this to the Eleventy Image v3.2.0 milestone Feb 2, 2024
@zachleat zachleat merged commit e17ff95 into 11ty:main Feb 2, 2024
@zachleat
Copy link
Member

zachleat commented Feb 2, 2024

Shipping with v3.2.0

zachleat added a commit that referenced this pull request Feb 2, 2024
@zachleat
Copy link
Member

zachleat commented Feb 2, 2024

This will be the minimumThreshold option and the default will be 1.25 to start.

When the original width is smaller than the desired output width, this is the minimum size difference
between the next smallest image width that will generate one extra width in the output.
e.g. when using widths: [400, 800], the source image would need to be at least (400 * 1.25 =) 500px wide
to generate two outputs (400px, 500px). If the source image is less than 500px, only one output will
be generated (400px).

@JaredReisinger JaredReisinger deleted the largest-non-upscale branch February 14, 2024 05:57
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.

Resizing Feature Request: output largest image without upscaling
3 participants