search

withastro / roadmap

Ideas, suggestions, and formal RFC proposals for the Astro project.
290 stars 29 forks source link

First-party support for a Picture component RFC #715

Closed Princesseuh closed 9 months ago

Princesseuh commented 11 months ago

Summary

The current <Image /> component and getImage APIs are great to show a single image, however it doesn't answer some of the common needs of the web, such as fallback formats and responsive images. This RFC hopes to define the addition of two features:

Links

jlarmstrongiv commented 11 months ago

@Princesseuh thank you for letting me try it out!

I’m getting a type error:

Type '{ src: ImageMetadata; widths: number[]; formats: ("avif" | "webp")[]; alt: string; }' is not assignable to type 'IntrinsicAttributes & Props'.
  Type '{ src: ImageMetadata; widths: number[]; formats: ("avif" | "webp")[]; alt: string; }' is not assignable to type 'IntrinsicAttributes & { title?: string | null | undefined; 'class:list'?: string | Record<string, boolean> | Record<any, any> | Iterable<string> | Iterable<...> | undefined; ... 199 more ...; src: string; } & { ...; }'.

I did not expect this warning:

Cannot use `widths` and `densities` at the same time. Using `densities`.

I figured I would be able to use widths and tack on a density. Say, 1440w @2x density. Will I have to define 2880w instead?

Another unexpected result is that it upscales images. I would expect that if a source image is only 1200px wide, it would not upscale the image to 1440px or 2880px. Instead, it should just link to the original as the max size. Is there a way to specify original as one of the widths too?

Lastly, while:

Automatic generation or API to make writing sizes easier

What do you think about sensible defaults? WordPress for example uses:

Anyway, just food for thought.

Princesseuh commented 11 months ago

@Princesseuh thank you for letting me try it out!

I’m getting a type error:

Type '{ src: ImageMetadata; widths: number[]; formats: ("avif" | "webp")[]; alt: string; }' is not assignable to type 'IntrinsicAttributes & Props'.
  Type '{ src: ImageMetadata; widths: number[]; formats: ("avif" | "webp")[]; alt: string; }' is not assignable to type 'IntrinsicAttributes & { title?: string | null | undefined; 'class:list'?: string | Record<string, boolean> | Record<any, any> | Iterable<string> | Iterable<...> | undefined; ... 199 more ...; src: string; } & { ...; }'.

Not sure about that one, probably just something I didn't finish.

I did not expect this warning:

Cannot use `widths` and `densities` at the same time. Using `densities`.

I figured I would be able to use widths and tack on a density. Say, 1440w @2x density. Will I have to define 2880w instead?

This is actually a limitation of HTML itself, you cannot mix pixel and width descriptors in the same srcset. Decided to follow that since it seemed the most intuitive

Another unexpected result is that it upscales images. I would expect that if a source image is only 1200px wide, it would not upscale the image to 1440px or 2880px. Instead, it should just link to the original as the max size. Is there a way to specify original as one of the widths too?

Hmm, we could definitely add a way to specify the original size as a widths, that should cover the first use case nicely. There are cases where you do actually want it to upscale, so it makes sense to support it.

Lastly, while:

Automatic generation or API to make writing sizes easier

What do you think about sensible defaults? WordPress for example uses:

  • Thumbnail: 150px square
  • Medium size: maximum 300px width and height
  • Large size: maximum 1024px width and height
  • Full size: full/original image size you uploaded

Still thinking about that one because every framework kinda has a different take on it and I'm not sure which one would be the best for Astro. Nuxt notably has an entire system where you set up screens and then there's a DSL to generate sizes, it's a lot.

For now I'm still in the camp for it being a non-goal, but would be happy to revisit in another proposal perhaps.

jlarmstrongiv commented 11 months ago

Not sure about that one, probably just something I didn't finish.

I was able to find the source of type error when the Image component released, but not sure about where this one is coming from.

This is actually a limitation of HTML itself, you cannot mix pixel and width descriptors in the same srcset. Decided to follow that since it seemed the most intuitive

Interesting. I usually use a library for this kind of thing, so that’s nice to know.

Hmm, we could definitely add a way to specify the original size as a widths, that should cover the first use case nicely. There are cases where you do actually want it to upscale, so it makes sense to support it.

I’ve been racking my brain, but I can’t think of a use case for that. I manually use AI upscalers if I need to upscale an image. Or, for things like pixel art, I usually use css image-rendering. It’d be great to have the option to disable that completely; otherwise, I’ll just be shipping a lower-quality image while using more bandwidth.

Still thinking about that one because every framework kinda has a different take on it and I'm not sure which one would be the best for Astro. Nuxt notably has an entire system where you set up screens and then there's a DSL to generate sizes, it's a lot.

For now I'm still in the camp for it being a non-goal, but would be happy to revisit in another proposal perhaps.

I’ll probably end up making a general Picture component wrapper with default settings then

Princesseuh commented 11 months ago

Hmm, we could definitely add a way to specify the original size as a widths, that should cover the first use case nicely. There are cases where you do actually want it to upscale, so I’ve been racking my brain, but I can’t think of a use case for that. I manually use AI upscalers if I need to upscale an image. Or, for things like pixel art, I usually use css image-rendering. It’d be great to have the option to disable that completely; otherwise, I’ll just be shipping a lower-quality image while using more bandwidth.

Hmm, I'm not necessarily against not having it, l think I might just not be sure how it would exactly manifest.

Let's say you have an original image that is 1280x720, so classic 720p.

You write this: widths: {[320, 640, 1280, 1920]}, what would you expect it to generate both in regards to actual images being generated and the srcset? Should it completely ignore the 1920 value? Should it lie and use a 1280x720 image with a 1920w descriptor?

Similarly, should densities be invalid without having also specified a width that is lower than the original image? Otherwise 2x will always require upscaling the image to provide a 2x.

Princesseuh commented 11 months ago

Published a new preview release of the implementation PR: https://github.com/withastro/astro/pull/8620 with the following changes that I collected from feedback on this PR:

I will be updating the RFC with those changes soon.

florian-lefebvre commented 11 months ago

Is it possible to apply the width | densities at the type level as well? Like

type PictureProps = AllTheProps & ({ widths: ...; } | { densities: ...; })
Princesseuh commented 11 months ago

Is it possible to apply the width | densities at the type level as well? Like

type PictureProps = AllTheProps & ({ widths: ...; } | { densities: ...; })

Hmm, yes I believe that would be possible!

jlarmstrongiv commented 11 months ago

@Princesseuh

Hmm, I'm not necessarily against not having it, l think I might just not be sure how it would exactly manifest.

You write this: widths: {[320, 640, 1280, 1920]}, what would you expect it to generate both in regards to actual images being generated and the srcset? Should it completely ignore the 1920 value? Should it lie and use a 1280x720 image with a 1920w descriptor?

If I wrote {[320, 640, 1280, 1920, "original"]} and it was an 1440x960, then it would ignore 1920 and generate srcsets for {[320, 640, 1280, "original"]}. Essentially, it would just ignore/skip values in widths and densities where the image isn’t large enough.

widths and densities no longer upscale the images

Anyway, thank you for disabling automatic image upscale 😄 I will try out the new prerelease again tomorrow—I’m going to Mt. Evans today 🏔️

carlcs commented 11 months ago

Thanks for working on this @Princesseuh! It’s all looking very promising. If I understand correctly, this doesn’t allow to produce images in different aspect ratios than the original one. Is this just out of scope for this implementation and will be added later? If that’s the case it might be worth adding a sentence on that in the non-goals section of the RFC.

Princesseuh commented 11 months ago

Thanks for working on this @Princesseuh! It’s all looking very promising. If I understand correctly, this doesn’t allow to produce images in different aspect ratios than the original one. Is this just out of scope for this implementation and will be added later? If that’s the case it might be worth adding a sentence on that in the non-goals section of the RFC.

Yes, this is out of scope for this proposal. This is what is meant by "Complex art direction with the built-in component" in the Non-goals section.

It could be added in the future, for sure! Right now our base image services don't support cropping, which is kinda required for this

carlcs commented 11 months ago

This is what is meant by "Complex art direction with the built-in component" in the Non-goals section.

I didn’t mean art direction (different source images or aspect ratios served using media queries) and I understand that this makes things complex. A very common use case though is to serve an image in a different aspect ratio than the one of the original image. You can of course use CSS to crop / hide parts of the image, but I would rather not send pixels that are never seen by anyone.

Princesseuh commented 11 months ago

This is what is meant by "Complex art direction with the built-in component" in the Non-goals section.

I didn’t mean art direction (different source images or aspect ratios served using media queries) and I understand that this makes things complex. A very common use case though is to serve an image in a different aspect ratio than the one of the original image. You can of course use CSS to crop / hide parts of the image, but I would rather not send pixels that are never seen by anyone.

Ah yes, you mean just for srcset, yeah this is not included in this proposal (again because it requires cropping). In the future if we support cropping, I'd definitely be open to an API where you can manually specify different shape ("variants") of the image. I'll update the RFC to make this more clear!

Princesseuh commented 11 months ago

Published a draft of docs for this feature here: https://github.com/withastro/docs/pull/4866

jlarmstrongiv commented 11 months ago

@Princesseuh

Is there a setting I need to use to avoid upscaling?

Princesseuh commented 11 months ago

@Princesseuh

  • [x] Docs look good
  • [x] TypeScript error is fixed
  • [ ] Avoid upscaling small images

Is there a setting I need to use to avoid upscaling?

Hmm, no it should normally work by default! I mainly tested with densities, so it's possible there's a mistake for widths. Is it possible that it's still generating the widths, but the image paths only refer to the right sizes?

(We'll of course do tests and stuff before actually releasing it)

jlarmstrongiv commented 11 months ago

Hmm, no it should normally work by default! I mainly tested with densities, so it's possible there's a mistake for widths. Is it possible that it's still generating the widths, but the image paths only refer to the right sizes?

Ahh, I was using widths—I believe it still includes and upscales widths larger than the image in the srcset

Princesseuh commented 11 months ago

Hmm, no it should normally work by default! I mainly tested with densities, so it's possible there's a mistake for widths. Is it possible that it's still generating the widths, but the image paths only refer to the right sizes?

Ahh, I was using widths—I believe it still includes and upscales widths larger than the image in the srcset

Hmm, weird. I'll take a look next week when I'm back to work, sorry for the inconvenience!

ShivamJoker commented 11 months ago

Awesome work guys. Waiting for this to be completed and merged! So that I can upgrade to Astro 3.0

Princesseuh commented 10 months ago

Posted a new preview release:

Unless something else comes up, this is likely to be the version that we'll have in experimental mode on next Thursday. Feedback is of course still welcome, we're just trying to get it into more hands.

Tanguy-Magnaudet commented 10 months ago

Hello,

Thank you for making this component available again.

I just upgrade to Astro 3 and it was easy to update the Picture component.

However, I encountered an error:

 MissingImageDimension: Missing width attribute for /content/uploads/anchor-text-html.png. When using remote images, both dimensions are always required in order to avoid CLS.

The component crashes if the width or height attributes are missing, even though standard HTML doesn't have this behavior.

Of course CLS is important, but I'm wondering if it's appropriate for Astro to enforce this. In the context of a blog article powered by a CMS, it's not always possible to predict media dimensions, and inputting arbitrary values seems counterintuitive.

What do you think about throwing the warning but still rendering the component ?

Princesseuh commented 10 months ago

Hello,

Thank you for making this component available again.

I just upgrade to Astro 3 and it was easy to update the Picture component.

However, I encountered an error:

 MissingImageDimension: Missing width attribute for /content/uploads/anchor-text-html.png. When using remote images, both dimensions are always required in order to avoid CLS.

The component crashes if the width or height attributes are missing, even though standard HTML doesn't have this behavior.

Of course CLS is important, but I'm wondering if it's appropriate for Astro to enforce this. In the context of a blog article powered by a CMS, it's not always possible to predict media dimensions, and inputting arbitrary values seems counterintuitive.

What do you think about throwing the warning but still rendering the component ?

This is the same behaviour as Image, so at this time it's intended when using the base services. In the future, we could probably have a fill attribute or something when you want to opt-in into CLS. It'd probably require some other changes though, since right now we kinda depend on width and height being present. Thank you for your feedback!

wtchnm commented 10 months ago

Hey, awesome work! It would be great if we could configure the default Picture formats, for example ['avif', 'webp'], as it was in the @astrojs/image integration, or even change the component defaults. I can help by making a PR if necessary!

Princesseuh commented 10 months ago

Hey, awesome work! It would be great if we could configure the default Picture formats, for example ['avif', 'webp'], as it was in the @astrojs/image integration, or even change the component defaults. I can help by making a PR if necessary!

That would be nice! Definitely something I'd be open to add. I assume people would love to see a defaultWidths too, since that's pretty common.

Princesseuh commented 10 months ago

Hello everyone!

We're entering the final phase of comments on this RFC. If you have any final feedback or concerns, please leave them here. Of course, this does not mean the feature is set in stone, but more so that this initial design makes sense. In the future, it is very possible that we would add some of the missing features here, such as art direction for instance.

Princesseuh commented 9 months ago

Thank you everyone for your comments! This Stage 3 proposal is now accepted, and the feature is available in Astro. We'll be improving and maintaining it for the future with fixes and new features (probably including some of the non-goals described in this RFC)