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

Add support for SVG images to  @astrojs/image #6118

Merged
merged 4 commits into from
Feb 15, 2023

Conversation

ggounot
Copy link
Contributor

@ggounot ggounot commented Feb 3, 2023

This fixes issue #6101.

Changes

Add support for SVG images to @astrojs/image:

  • add '*.svg' module declaration;
  • add svg in InputFormat and OutputFormat;
  • update SharpService.transform() and SquooshService.transform() to directly return the input image if the format is svg.

This allows importing SVG files and displaying them through <Image />.

Sharp and Squoosh cannot output SVG so no transformation is performed if the output format is SVG.

The SVG's metadata can be read and used to generate the right attributes in the resulting <img />.

Testing

Added an SVG image in the basic-image fixture and tests for it in image-ssg.test.js, image-ssr-build.test.js and image-ssr-dev.test.js.

Docs

Added information on SVG format usage in @astrojs/image's README.md.

@ggounot ggounot requested a review from a team as a code owner February 3, 2023 16:28
@changeset-bot
Copy link

changeset-bot bot commented Feb 3, 2023

🦋 Changeset detected

Latest commit: a353249

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Feb 3, 2023
@ggounot ggounot changed the title Astrojs image svg Add support for SVG images to  @astrojs/image Feb 3, 2023
**Default:** `undefined`
</p>

The output format to be used in the optimized image. The original image format will be used if `format` is not provided.

This property is required for remote images only, because the original format cannot be inferred.

The `svg` format is only valid when the original image is already in SVG format. The image itself won't be processed but the resulting `<img />` will have the right attributes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tempted to make this a note, since this format works differently from all the others. When using the svg option:

  • the original image must be in SVG format already
  • the image integration cannot transform a non-SVG image into an SVG
  • remote SVG images are not "processed" (does this mean optimized? or does this mean "changed to this format"? Processed could mean different things.) If the image is not optimized by the integration, that's another difference when setting the type to svg.

This seems like enough to call out as something that really is "not like the others" and might even by helpful to mention when/why you would choose to use the image integration for an svg, instead of choosing just an <img> tag. It's not clear to me from this that the image integration does much for you here. 😄

Copy link
Contributor Author

@ggounot ggounot Feb 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @sarah11918 for looking into this. :)

Yes, by "processed" I meant transformed/optimized.

The image integration not only transforms the images but also reads the width and height of the image to put them into the width and height attributes of the <img> tag, which optimizes the cumulative layout shift (CLS).

Transformation to SVG is not possible but CLS optimization is, so the image integration is still useful for SVG images.

Moreover, making <Image /> accept all image formats that work with <img /> allows the developer to not worry about SVG's specific case when using a bunch of images of various formats.

I hope I was clearer. I can add more details to the README if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the README. Hope it's better now :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The image integration not only transforms the images but also reads the width and height of the image to put them into the width and height attributes of the <img> tag, which optimizes the cumulative layout shift (CLS).

@ggounot You are absolutely right; I am reading more and more about how specifying width and height attributes on <img> tags is so important now.

(I was also trying to do this on a site built with Astro, but as all images (binary and SVGs) are already optimized, I was only trying to automatically extract the width and height of each image and add them to the <img> tag. BTW, what are you using to extract the SVG width and height in your code?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@astrojs/image extracts image metadata using the image-size package. This is done automatically when an image is imported through a Vite plugin. Then the <Image /> component sets the width and height attributes using this metadata + the loading and decoding attributes.

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the wait, we've been discussing this internally making sure it's something we wanted.

This looks great, thank you very much for doing all the work (code, documentation and tests? In one PR? That's awesome)

@Princesseuh Princesseuh merged commit ac3649b into withastro:main Feb 15, 2023
@astrobot-houston astrobot-houston mentioned this pull request Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants