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

srcsetMaxWidth returns incorrect image sizes #407

Closed
almeric opened this issue Jun 7, 2024 · 6 comments
Closed

srcsetMaxWidth returns incorrect image sizes #407

almeric opened this issue Jun 7, 2024 · 6 comments
Labels

Comments

@almeric
Copy link

almeric commented Jun 7, 2024

Describe the bug

image.optimized.srcsetMaxWidth(576) returns wrong image sizes. I've narrowed it down to here:

https://github.com/nystudio107/craft-imageoptimize/blob/v5/src/models/OptimizedImage.php#L642-L644

$set and $this->variantSourceWidths are not ordered the same. $set starts with the smallest size, while variantSourceWidths starts with the largest size. Therefore you can't slice using the index.

Example (urls omitted):

$set:

{
  "320": "url",
  "480": "url",
  "576": "url",
  "768": "url",
  "1280": "url",
  "1440": "url",
  "1920": "url",
  "2048": "url"
}

variantSourceWidths:

[
  "2048",
  "1920",
  "1440",
  "1280",
  "768",
  "576",
  "480",
  "320"
]

To reproduce

Steps to reproduce the behaviour:

   {{ image.optimized.srcsetMaxWidth(576) }}

Returns images with sizes 1440, 1920, 2048

Expected behaviour

Images with sizes 320, 480, 576

Versions

  • Plugin version: 5.0.1
  • Craft version: 5.1.9
@almeric almeric added the bug label Jun 7, 2024
@almeric
Copy link
Author

almeric commented Jun 14, 2024

@khalwat any chance you can take a look at this?

@khalwat
Copy link
Contributor

khalwat commented Jun 14, 2024

Yessir, I am away at the moment, but I return on Saturday. It's on my list!

@khalwat
Copy link
Contributor

khalwat commented Jun 14, 2024

Addressed in the above commits

Craft CMS 3:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-imageoptimize": "dev-develop as 1.6.55”,

Then do a composer clear-cache && composer update

…..

Craft CMS 4:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-imageoptimize": "dev-develop-v4 as 4.0.9”,

Then do a composer clear-cache && composer update

…..

Craft CMS 5:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-imageoptimize": "dev-develop-v5 as 5.0.2”,

Then do a composer clear-cache && composer update

@khalwat khalwat closed this as completed Jun 14, 2024
@almeric
Copy link
Author

almeric commented Jun 17, 2024

Thanks @khalwat! Unfortunately it's not completely fixed:

Using ksort for variantSourceWidths won't work as it's already sorted by key, you'll have to sort by value I guess?

See debug info:

Screenshot 2024-06-17 at 13 24 51

ksort won't have any effect here.

@khalwat
Copy link
Contributor

khalwat commented Jun 17, 2024

My bad... Addressed it in the dev branches, do this again to test it. Here's the commit:

db1c1fe

Craft CMS 3:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-imageoptimize": "dev-develop as 1.6.55”,

Then do a composer clear-cache && composer update

…..

Craft CMS 4:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-imageoptimize": "dev-develop-v4 as 4.0.9”,

Then do a composer clear-cache && composer update

…..

Craft CMS 5:

You can try it now by setting your semver in your composer.json to look like this:

    "nystudio107/craft-imageoptimize": "dev-develop-v5 as 5.0.2”,

Then do a composer clear-cache && composer update

@almeric
Copy link
Author

almeric commented Jun 19, 2024

Thanks! All is working as expected now!

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

No branches or pull requests

2 participants