search

lorenzodejong / next-sanity-image

Utility for using responsive images hosted on the Sanity.io CDN with the Next.js image component.
MIT License
149 stars 20 forks source link

Make if more clear that default layout is "responsive" and not "intrinsic" #7

Closed wenche closed 3 years ago

wenche commented 3 years ago

Hi, and thanks for a great work 👏

Is it possible to highlight more in the README that this library change the default layout from intrinsic to responsive? We find it very confusing now since the Next.js API documents intrinsic as default and it took quite a long time before we realized that this is changed to responsive by this library 😅

lorenzodejong commented 3 years ago

There's documentation about this on the type for the return value (the props you pass to the Next.JS Image component): https://github.com/bundlesandbatches/next-sanity-image#return-value-usenextsanityimageprops. Besides this return type the introduction about the library also highlights this is meant for use with responsive images: https://github.com/bundlesandbatches/next-sanity-image#next-sanity-image. How would you suggest to improve this documentation?

wenche commented 3 years ago

Actually by mentioning it in the gotcha :) I might have been fooled by the name next-sanity-image and I didn't actually realize it was just for layout responsive.

lorenzodejong commented 3 years ago

To be fair this is not really a gotcha in the library, as this is actually the intended implementation. There's not really a point in creating a library for non-responsive images from the Sanity CDN, since it's easier to just use @sanity/image-url directly to build your URL.

mikestopcontinues commented 3 years ago

intrinsic images are still responsive. They only resize down. The closer you stick to the next/image api, the easier it'll be for users to reason about the library.

lorenzodejong commented 3 years ago

Looking at the implementation of Next.js Image it would probably be possible to remove the layout prop from the return value and enforce the implementation to specify the layout. In case the layout is intrinsic, the src prop will be used. If the layout is responsive the loader prop will be used. I'll check if this implementation strategy works. If this is the case this would however result in a breaking change.

lorenzodejong commented 3 years ago

Created a PR with the base implementation, it looks like this implementation works as intended. It's a bit different from my previous comment as this implementation utilizes the loader callback in every layout scenario.

Will do some final tests and write unit tests before integrating this feature.

mikestopcontinues commented 3 years ago

Very cool! This is a great add!

lorenzodejong commented 3 years ago

I've just released version 3.0.0 on npm, would be glad to hear if this solves your use case(s). If there's any issue with this implementation, feel free to open a bug report!