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

Check the width against the srcset values #660

Open
ludoboludo opened this issue Nov 8, 2022 · 2 comments
Open

Check the width against the srcset values #660

ludoboludo opened this issue Nov 8, 2022 · 2 comments
Labels
e:3 Effort: 3 enhancement New feature or request good first issue Good for newcomers linter Check related issues

Comments

@ludoboludo
Copy link

Is your feature request related to a problem? Please describe.

When you output an image like this:

{{ section.settings.image_2 | image_url: width: 3840 | image_tag:
      loading: 'lazy',
      width: section.settings.image_2.width,
      height: image_height_2,
      class: image_class_2,
      sizes: sizes,
      widths: '375, 550, 750, 1100, 1500, 1780, 2000, 3000, 3840',
      alt: section.settings.image_2.alt | escape
 }}

The image_url: width: value is going to limit the what widths you can output/generate. So ideally the image_url: width: value should be similar to the highest value declared in the widths:.

Describe the solution you'd like

There could be a warning showing up when the value is different or lower 🤔

Describe alternatives you've considered

Manual checking 😅

Additional context

This is something we ran into in Dawn. We realized we were using 1500 on the image banner image and it would never output an image bigger than 1500 despite having some widths set up to 3840.

@charlespwd charlespwd added enhancement New feature or request e:3 Effort: 3 good first issue Good for newcomers linter Check related issues labels Nov 9, 2022
@bakura10
Copy link

bakura10 commented Jan 9, 2023

@ludoboludo , the proper fix is to make the width optional for image_url and make sure it generates the highest possible value:

{{ section.settings.image_2 | image_url | image_tag: xxx }}

The width should be implicitly the image max width. I remember we had this discussion already, has it been refused?

@ludoboludo
Copy link
Author

Hey @bakura10, I think there might be a blocker to do with potential future features. Not too sure but I'll double check with the team to confirm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e:3 Effort: 3 enhancement New feature or request good first issue Good for newcomers linter Check related issues
Projects
None yet
Development

No branches or pull requests

3 participants