bigcommerce / paper-handlebars

Paper plugin for rendering via Handlebars.js
BSD 4-Clause "Original" or "Old" License
12 stars 36 forks source link

Image and SrcSet helpers don't respect custom srcsets #104

Open richardweaver opened 4 years ago

richardweaver commented 4 years ago

I've been trying to optimize image sizes using the getImageManagerImageSrcset handlebars helper. Looking at the documentation here I should be able to specify custom sizes including inherent sizes and pixel densities: https://developer.bigcommerce.com/stencil-docs/reference-docs/handlebars-helpers-reference

However, as far as I can see there's nothing in the code other than the default srcset ever applied and no parameter(s) anywhere that expect custom sizes to be applied? https://github.com/bigcommerce/paper-handlebars/blob/master/helpers/lib/getObjectStorageImage.js

Am I misinterpreting the code or is the documentation wrong?

bookernath commented 4 years ago

Hey @richardweaver - thanks for raising this.

With our first "srcset" helper which is {{getImageSrcset}}, we saw no next-to-no usage of custom sizes. So when creating {{getImageManagerImageSrcset}} and {{getContentImageSrcset}} we omitted the option to specify custom sizes.

The docs are a bit misleading in that regard - so I'll work with @bigcommerce/dev-docs to get those corrected.

Can you talk a bit more about your use case for custom sizes? I'd be happy to consider adding the option to the helper if it's useful to you.

In the interim, you could consider using {{getImageManagerImage}} and {{getContentImage}} to build your own srcset like this:

<img srcset="{{getImageManagerImage "asset.jpg" width=100}} 100w, {{getImageManagerImage "asset.jpg" width=200}} 200w" />

richardweaver commented 4 years ago

Thanks @bookernath for getting back to me. The main use case was to have more fine-grained control on image sizes. Because of the way the responsiveness of our site works we know our maximum image size for some slots, and also the key "step" sizes, so it appeared to make sense to specify them in the srcset. Although it's only a small saving we're quite image heavy so it adds up.

For example, if we need an image of 1960px wide, because this is just over 1920w with the default size set the browser will use the 2560px wide image which could cost 100s kb bandwidth

Perhaps one alternative would be to be able to specify maximum (and maybe minimum) size for the srcset?

steve-ross commented 2 years ago

I'd love this feature, really annoying as I thought the docs supported this as well. Use case: there is a massive jump in the 'default sizes' and making a helper for every option is a PITA.

use case for us is the ability to deliver images at closer breakpoints instead of the huge jump between the default sizes apparently that was just for 'hero' or 'full width' images?

"srcset_sizes":{
    "thumb_sm": {
      "50w": "50w",
      "80w": "80w",
      "120w": "120w",
      "160w": "160w"
    },
    "thumb_med": {
      "100w": "100w",
      "140w": "140w",
      "200w": "200w",
      "240w": "240w",
      "280w": "280w"
    },
    "four_cols": {
      "340w": "340w",
      "360w": "360w",
      "380w": "380w",
      "420w": "420w",
      "460w": "460w",
      "500w": "500w"
    },
}

The alternative is a pain:

<img alt="{{alt}}" class="lazy placeholder{{#if class}} {{class}}{{/if}}" 
  src="{{getContentImage image width=50 }}" 
  data-src="{{getContentImage image width=360 }}"
  data-srcset="{{getContentImage image width=360 }} 360w,
  {{getContentImage image width=480 }} 480w,
  {{getContentImage image width=550 }} 550w,
  {{getContentImage image width=640 }} 640w,
  {{getContentImage image width=800 }} 800w,
  {{getContentImage image width=1024 }} 1024w,
  {{getContentImage image width=1260 }} 1260w,
  {{getContentImage image width=1420 }} 1420w"
  data-sizes="{{#if sizes}}{{sizes}}{{else}}100vw{{/if}}" />