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

Image size set to 1w if sizes is set to 100vw #1433

Open
stijns96 opened this issue Aug 21, 2024 · 7 comments
Open

Image size set to 1w if sizes is set to 100vw #1433

stijns96 opened this issue Aug 21, 2024 · 7 comments

Comments

@stijns96
Copy link

Somehow 100vw will be converted to a 1w and 2w url and I don't understand why.

Local

<img
  sizes="(max-width: 640px) 100vw, (max-width: 768px) 50vw, 400px"
  srcset="
    /_ipx/w_1/twitter-image.png     1w,
    /_ipx/w_2/twitter-image.png     2w,
    /_ipx/w_320/twitter-image.png 320w,
    /_ipx/w_400/twitter-image.png 400w,
    /_ipx/w_640/twitter-image.png 640w,
    /_ipx/w_800/twitter-image.png 800w
  "
  src="/_ipx/w_800/twitter-image.png"
/>

Storyblok

<img
  sizes="(max-width: 640px) 100vw, (max-width: 768px) 50vw, 400px"
  srcset="
    https://a.storyblok.com/f/23194/3000x2000/09das0980/image.png/m/1x0     1w,
    https://a.storyblok.com/f/23194/3000x2000/09das0980/image.png/m/2x0     2w,
    https://a.storyblok.com/f/23194/3000x2000/09das0980/image.png/m/320x0 320w,
    https://a.storyblok.com/f/23194/3000x2000/09das0980/image.png/m/400x0 400w,
    https://a.storyblok.com/f/23194/3000x2000/09das0980/image.png/m/640x0 640w,
    https://a.storyblok.com/f/23194/3000x2000/09das0980/image.png/m/800x0 800w
  "
  src="https://a.storyblok.com/f/23194/3000x2000/09das0980/image.png/m/800x0"
/>
@stijns96
Copy link
Author

Meanwhile I get why. There is no breakpoint to match, but still this should work. I like the way the srcset is build up from the sizes, but sometimes it's works against it.

Sometimes it would he better to just have an array of widths that the srcset should be build of.

Most of the time you only need a px size if the container reaches it's max width. Everything below should use vw

@dbismut
Copy link

dbismut commented Sep 4, 2024

It's actually really weird and it's very strange that this issue doesn't have more upvotes as it basically affects all providers.

The problem lies in that logic:

for (const entry of input.split(/[\s,]+/).filter(e => e)) {
const s = entry.split(':')
if (s.length !== 2) {
sizes['1px'] = s[0]!.trim()
}
else {
sizes[s[0]!.trim()] = s[1]!.trim()
}
}

For a size input such as "sm: 100vw lg:480px" this function should return a sizes object such as:

sizes = {
  lg: '480px',
  sm: '100vw',
}

But whenever a size is provided without a media query, this function sets it to 1px in size. So: "100vw lg:480px" will return:

sizes = {
  '1px': '100vw',
  lg: '480px',
}

Then further there is some logic that converts the keys to their actual screens values, so the object will be transformed with actual pixel values instead of screen keys, let's says like this:

sizes = {
 '1px': '100vw',
 1280: '480px',
}

And finally with default densities being [1, 2] the sizes object will multiply its keys by x2 hence the 1w and 2w that are so weird. Then it seems also that the package doesn't generate all the srcsets matching the screens defined in nuxt config.

Anyway long story short:

  • a size without a media query is interpreted incorrectly
  • srcset doesn't include all breakpoints.

This is a major inconvenience and make this package pretty much useless without a fix.

@stijns96
Copy link
Author

stijns96 commented Sep 4, 2024

Hi @dbismut ,

Nice explenation! I already moved away from this package... it indeed makes it useless now.

@dbismut
Copy link

dbismut commented Sep 4, 2024

Do you have a drop in replacement?

@stijns96
Copy link
Author

stijns96 commented Sep 4, 2024

Do you have a drop in replacement?

Well, I created my own image component. It's fully based on the Storyblok image service

@iBobik
Copy link

iBobik commented Oct 5, 2024

Do we really need to specify 100vw on the start? Isn’t enough this?
xs:100vw sm:100vw md:100vw lg:100vw xl:100vw 2xl:100vw

(Related: #267 )

@dbismut
Copy link

dbismut commented Oct 6, 2024

Of course this can work but it is fastidious, it's like setting media queries for every breakpoint for each responsive class, that doesn't make sense.

Plus it is supposed to work from the docs.

Also nuxt/image is confusing breakpoints and screen variants which are two different things.

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

No branches or pull requests

3 participants