Built-in SVG Components#1035
Conversation
lorenzolewis
left a comment
lorenzolewis
left a comment
There was a problem hiding this comment.
Super excited to see this one move forward! Just a quick note on the framing of this when it gets to docs.
Also, for Sprites, do you think explaining the benefits makes sense, or is this perhaps only an implementation detail? I'm not familiar at all so just curious.
|
thanks, for hard working and supporting <use> as I requested |
|
Can you explain how you are implementing the |
|
If a developer removes an SVG element from the DOM (using client-side JS) that holds the symbols (I.e. the source), will that break other SVGs on the page using that symbol? |
Yes, that will break the rest of the SVGs using that symbol. This is one of the nuances to using this sprite setup. Another option would be to put all the symbols into a single SVG that could be placed on the page (perhaps in a layout for simplicity). This would avoid that nuance but would complicate things in the future from a lazy loading perspective, generating the build, and would a slightly more code (though this is probably negligible) |
The user doesn't need to author the SVG in any certain way for it to be used as a sprite. The sprite behavior is:
All future SVGs will not create and add the |
I can imagine this will present some difficult support scenarios, especially if adding a framework-specific way on top of this is a future goal as you've mentioned in the original RFC. Would there be any downside to having a separate SVG defining the Thinking outside the box a bit here, I could imagine a setup like this:
---
import { SvgSymbol } from 'astro:assets' // naming tbd
import Logo from '../assets/logo.svg';
import Layout from './layouts/BaseLayout.astro'
---
<Layout>
<SvgSymbol />
<Logo />
<h1>I'm a cool page</h1>
</Layout>With the above case, a user is specifically opting-in to the sprite behavior and it is clear where it is used. My mind is thinking a bit on how View Transitions were implemented: https://docs.astro.build/en/guides/view-transitions/#adding-view-transitions-to-a-page |
|
There are more gotchas when using Sprites which is why we should make that opt-in rather than the default. Even with using a Sprite component to hold the aggregate of SVGs on the page.
As I was mentioning previously, this does complicate the implementation, could prove hard for SSR lazy-loading later, and very slight increase in code size. That being said, I think it might be worth it for the trade-off for avoiding an edge-case on the Sprite usage and possible improvement for framework usage. Would love @natemoo-re's thoughts on it. |
|
Just wanted to say that doing this: ---
import ThumbIcon from '../assets/icons/ui/thumbs-up.svg?raw';
---
<Fragment set:html={ThumbIcon} />Has been pretty great for me. I like the idea of "it just works" rather than having to find this in the docs. But I'd rather not overcomplicate things if it creates problems elsewhere. |
|
A QoL thing: It'd be nice if I can style the svg without having to use a |
I'm not quite sure I'm following. Can you provide an example or for details? |
Yes! ---
import RandomSVG from ".@src/imgs/duck_icon.svg";
---
<RandomSVG />
<style>
svg { /* This would target the svg element that this results in. */ }
</style> |
I'm not sure there is much we can do about that. The styling is primarily leaning on the feature-set of Astro. You could use the |
|
I just checked the documentation for this feature: https://5-0-0-beta.docs.astro.build/en/reference/experimental-flags/svg/. I would love to use the feature in sprite mode. Example: theme switcher https://github.com/web-standards-ru/web-standards.ru/blob/main/src/includes/scheme-switcher.njk using SVG sprite https://github.com/web-standards-ru/web-standards.ru/blob/main/src/images/sprite.svg. Resulted code should be: Currently, I don't understand:
|
If you are wanting to switch all SVGs to use the {
experimental: {
svg: { mode: 'sprite' }
}
}This will automatically create a sprite and allow you to reuse the symbols.
You can just pass component props like you normally would. It will receive all props you pass. Docs ---
import Moon from '~/assets/moon.svg'
import Duo from '~/assets/duo.svg'
import Sun from '~/assets/sun.svg'
---
<section class="scheme-switcher" aria-label="Цветовая тема">
<button class="scheme-switcher__button" type="button" value="dark" aria-pressed="false" title="Тёмная">
<Moon class="scheme-switcher__icon" aria-hidden="true" width="20" height="20" />
<span class="visually-hidden">Тёмная</span>
</button>
<button class="scheme-switcher__button" type="button" value="auto" aria-pressed="false" title="Системная">
<Duo class="scheme-switcher__icon" aria-hidden="true" width="20" height="20">
<span class="visually-hidden">Системная</span>
</button>
<button class="scheme-switcher__button" type="button" value="light" aria-pressed="false" title="Светлая">
<Sun class="scheme-switcher__icon" aria-hidden="true" size="20" title="Светлая" />
<!-- you can use `size` instead of specifying both `width` and `height` -->
<!-- you can use `title` instead of adding a `span` with the text -->
</button>
</section> |
|
@stramel can you update the RFC to explain the |
My project has both individual SVGs, such as the project logo, and SVG sprites. I would like to work with SVGs in both formats at the same time, rather than either single SVGs or sprites. This is currently impossible, as far as I understand? |
This is possible as well, you can specify a default |
mayank99
left a comment
mayank99
left a comment
There was a problem hiding this comment.
This is great to see! I used to have to save my SVGs as .astro and spread Astro.props manually, whereas now I can keep using .svg, with props automatically forwarded. 🙌
I have three small pieces of feedback which I've left as comments below.
| ### ARIA Attributes | ||
|
|
||
| - **Role Attribute:** By default, Astro could set `role="img"` on SVGs when they are used for non-decorative purposes. | ||
| - **Title Element:** If the `.svg` file needs and accessible title/label, Astro can inject a `<title>` element. | ||
|
|
||
| **Example:** | ||
|
|
||
| ```astro | ||
| --- | ||
| import Logo from '../assets/logo.svg' | ||
| --- | ||
|
|
||
| <!-- Pass an accessible title that will be injected --> | ||
| <Logo title="Company Logo" /> | ||
| ``` | ||
|
|
||
| This would generate the following output: | ||
|
|
||
|
|
||
| ```astro | ||
| <svg width="24" height="24" role="img"> | ||
| <title>Company Logo</title> | ||
| <!-- SVG Content --> | ||
| </svg> | ||
| ``` | ||
|
|
||
| While this proposal offers strong defaults for accessibility, it is essential to give developers full control. Any automatically generated accessibility attributes (aria-label, role, etc.) should be overridable by the developer at the component level, ensuring that specific use cases or custom behaviors can be supported. |
There was a problem hiding this comment.
I don't think role="img" should be added by default.
When the SVG is not labelled, it would be interpreted as "image" (similar to an image without alt text), which is unhelpful. Further, when aria-hidden is specified from outside, I would definitely expect the role="img" to not be present.
Also, converting the title attribute into a <title> element feels a bit surprising as well. Is there a way to opt out of this?
There was a problem hiding this comment.
This is a good point. Our goal was to provide strong defaults for a11y. This (role="img") and <title> come over from astro-icon. From everything I have read, role="img" and <title> is a solid default for svgs.
Not sure how we could make the DX better here. @natemoo-re might have some insight on this as well.
references:
mdn
accessible SVGs
dequeue
w3c
css tricks
There was a problem hiding this comment.
From everything I have read,
role="img"and<title>is a solid default for svgs.
But it's not the default currently. By default, only role="img" is added. The title is optional.
And while role="img"+<title> is fine for labelling a static SVG, my comment is about the implementation in Astro.
- If the SVG contains meaningful content inside it,
role="img"may hide it by default. This is surprising behavior. - The
titleattribute gets confusingly converted into a<title>element. This is surprising behavior.- The
titleattribute is widely discouraged, so developers may feel uneasy using it and leave the SVG unlabelled.
- The
It may have been fine for a third-party library like astro-icon, but a framework should do the least surprising thing. So I would suggest making role="img" opt-in, and allow <title> to be passed as children.
---
import MySvg from "somewhere.svg";
---
<MySvg role="img">
<title>The label</title>
</MySvg>This is more predictable, more flexible, and also addresses my point above where both role="img" and aria-hidden="true" were present.
(Side note: in my personal experience, it's far more common to hide the SVG using aria-hidden="true" and instead make sure to label the encompassing element. The label is often dependent on the surrounding context, and the tooltip added by <title> is usually undesirable as well.)
There was a problem hiding this comment.
Agree with @mayank99 here. For various reasons I'll almost always have markup that looks something like the following (abbreviated for clarity):
<span>
Text label
<svg aria-hidden="true">...</svg>
</span>(Substitute button, a, etc. for span as appropriate, and potentially wrap the label in visually-hidden class if necessary.)
In fact one of the most common accessibility remediations I've had to apply to sites using astro-icon is going through adding aria-hidden="true" to everywhere the icon component is used.
There was a problem hiding this comment.
I do think I agree with you. Would still like to hear @natemoo-re and @matthewp 's perspectives as well.
There was a problem hiding this comment.
Tested and aria-labelledby does make it work. No role needed. This seems like a pretty good combination to me, you get:
- Voiceover
- Mouse hover tooltip
- Content in the page so translation works
And by omitting role="img" it doesn't announce it to be an image. What are the downsides to doing this?
There was a problem hiding this comment.
@stramel As no one is happy with the current behavior, if you wanted to remove this from the RFC for now, while we continue to try and reach a consensus on a direction, that would be fine with me.
There was a problem hiding this comment.
Not sure it's too helpful, but I would not expect an SVG integration to muck with a11y settings as a general-purpose tool. astro-icon makes more sense since it's an icon library, but as a framework integration for svgs as icons and as illustrations, I would expect control to be handed to me as the developer to implement my SVG's correctly. I'm not sure there's a one-size fits all approach that works for all usecases and doesn't step on toes.
Maybe an approach closer to <Image> where alt is a required prop would be prudent? Could write custom lints that require title or aria-label or aria-hidden to be set, but that might be a bit complicated.
There was a problem hiding this comment.
Thanks for the breakdown of alternatives! I think we're getting to the heart of the issue now, and that's about use-cases and design patterns. I have thoughts to drill down further, but first I want to hit on this point which I think is unrelated:
I’m not saying no-one’s allowed to use <title>. I’m saying <title> is not a good enough method to enable using it in a nonstandard way, which is a way of promoting this usage. Want to add it to your SVG files? Absolutely. Go ahead!
I don't buy the non-standard argument some have made. Of course it's non-standard, but so is importing an SVG, so are Titlecase components, so is
.astro. syntax. This is not a standards body, we're defining our own API.
There's a nuanced point that's being talked past here which is that by making a place for it in the documentation you are promoting it. In this case it is about best and better, and not about what's good or "seemed like a good idea (at the time)". I wish to tell you how frustrating it is when a developer decides that they know better than the standard implementation and unexpected things happen. I lose several hours. Clients lose thousands of dollars.
You are making your own API so you become a standards body.
This is important nuance to understand here. You may not be the standard--you are not WHATWG--, but you become a standard: you are Astro, and this is an RFC.
I think this nuance has been missing from a lot of Astro's perspective of late, and I'm saying something about it because it's pushing me and other developers away from the platform I love.
I don't think there's an expectation of props mapping directly to
<svg>element props. We have lots of components in Astro core that have their own props APIs. The<Code />component doesn't map directly to a<code>element, nor does<Image />map to an<img />. Generally speaking, components define their API in frameworks.That this particular component spreads unknown props onto the
<svg>element is a positive thing that makes it more powerful; but it shouldn't be a constraint we place on ourselves not to make the API better where appropriate.
This SVG implementation is important to me as I've been an advocate of SVG for over 10 years now. It's going to make or break my continued advocacy and evangelization of the platform to my clients and colleagues.
SVG and Web Components are the future of the internet whether we like it or not, and so we should strive to support the standard and not do unexpected behavior. When we do, it needs to be well documented and there needs to be a strong compelling reason for deviating from standards.
This is a tenet of the modern web won with blood, sweat, and tears. I mean that literally. As the old guy in the room who had to support IE5.5, you want to support standards as much as possible and you want to not be special. If you don't believe me, just look at "quirks mode" or Adobe flash.
There was a problem hiding this comment.
I'm wondering if there's a different approach that we could take where we don't necessarily expose the inner workings but instead expose what developers are looking for, which is fitting their use-case. Having a prop that allows developers to opt-in to a easy setup for what they're looking for might be a better option. These examples will likely be bad but hopefully get my point across.
as="icon"appliesrole=imgand requires alabelas="hidden'appliesaria-hidden=true
and have an option for the user to opt-out and control the a11y themselves.
| ### Sprite | ||
|
|
||
| An SVG Sprite is when an `<svg>` element defines multiple SVGs (or symbols) using the `<symbol>` element and attaching a unique identifier to each SVG as an `id`. These are then able to be referenced using the `<use>` element with an `href` attribute that points to the `id` defined on the `<symbol>` element. | ||
|
|
||
| When a `.svg` file is first imported and rendered in Astro, the system will convert the file into a `<symbol>` element. This will be inserted into the `<svg>` element. This approach ensures that all subsequent renders of that `.svg` can use a `<use>` element, referencing the ID of the initial `<symbol>`. |
There was a problem hiding this comment.
It seems like setting mode: "sprite" currently only inlines SVG symbols into the current page. This is fine as a default, but what would be ideal is if an external sprite sheet was generated, so that it could be cached and reused between pages. (Maybe Astro could even detect if SVGs are used in multiple pages? Although, that sounds a bit complicated.)
The external .svg file can be preloaded as well (either automatically when opting into this mode, or by providing developers with the URL).
As an added benefit of externalizing the sprite sheet, it would become easier to use SVGs in framework components. Imagine a component-agnostic API like this:
import { getSvgHref } from "astro:assets";
<svg>
<use href={getSvgHref("../icons/heart.svg")} />
</svg>External sprite sheets is largely my preferred approach of using SVGs today. I know framework support is listed as a "future" goal, but I'm mentioning this now because the decisions made today will likely have an impact on it. "SVGs-as-components" in client JS frameworks can be useful in advanced cases (like animation) but is usually an anti-pattern.
This can always be added in the future and doesn't need to block the feature from shipping today, but maybe the configuration option should be renamed from "sprite" to something like "inline-sprite"?
I did see your earlier comment related to this as well.
There was a problem hiding this comment.
TBH I'm not overly happy with the mode or sprite functionality. I added mode to allow for more flexibility, such as external sprites, sprite sheet, and the inline sprites (current) for the future.
I have a few thoughts that I need to tackle.
I'm also not super happy with the framework SVG story but it currently follows the standard in Astro. Hoping some of my thoughts will help with this as well.
I would love to have a sprite component that aggregates the SVGs into the component. Allowing for external or embedded.
We also discussed maybe something that just exports a use so that you could create your own sprites.
Across pages external is a must.
I'll try to summarize my thoughts for improving this and the future.
stramel
requested review from
delucis,
jasikpark,
mayank99,
natemoo-re and
tanishqmanuja
tanishqmanuja
left a comment
tanishqmanuja
left a comment
There was a problem hiding this comment.
If you measure the progress only keeping the Goals in mind. This looks food but still there is no native support for dynamic import or making multiple imports easy :( I mean honestly no one wants 10+ svg import lines at the top of the file for example say in a brands pages.
Dynamic import is supported 🙂 WRT the SVG imports, it isn't too hard to throw together an Icon component that utilizes dynamic import to associate a name with a file. |
|
Huge shout out to @stramel for getting this proposal across the line 👏 And thanks to everyone who provided feedback. Really valuable especially to the have the expert accessibility perspectives as part of this discussion. |
@florian-lefebvre thank you. Unfortunately I'm not clear on the PR processes here, and I'm not clear on the correct way to implement this either. The one removed from PR #13488 commit a513941 was an alias (and that was the reason for its removal): // packages/astro/src/assets/utils/svg.ts
import type { AstroComponentFactory } from '../../runtime/server/index.js';
export type SVGComponent = AstroComponentFactory;
// packages/astro/src/types/public/index.ts
export type { SVGComponent } from '../../assets/utils/svg.js';Personally though, I prefer add this to my projects, as it's not an alias: export type SVGComponent = typeof import('astro/client/*.svg').defaultNote to others: I add it without the |
|
|
|
Is |
|
IMO the important part is that the props are only for svg attrs instead of |
|
Ah right, good point. Then yeah probably good to expose it 👍 |
You mean like Maybe the type should be Astro component then? I'm not sure what that type is that right now. Thank you for moving this discussion forward! |
|
Once we figure out where to put the type, I can help you land the PR 🚀 |
|
@ADTC alright so here is what you can do:
We'll have to check if it can be used anywhere else but that should be a good start! |
|
@florian-lefebvre for the first one, am I correct to do this? export type SvgComponent = ((_props: Props) => any) & ImageMetadataOr is there something more specific than Also, do we prefer the capitalization
|
|
I think it should be the following export type SvgComponent = (props: astroHTML.JSX.SVGAttributes) => anyI don't think we have anything better than Let's start with that, we can discuss on the implementation PR directly 👍 |
|
I am currently using it like this https://github.com/tanishqmanuja/astro.portfolio/blob/main/src%2Fcomponents%2Fbrand%2FBrandLogo.astro#L4 |
|
@florian-lefebvre Oh yeah, I forgot to mention this part: 😄 type Props = astroHTML.JSX.SVGAttributes;I will go see about that PR. PS: @tanishqmanuja thanks for sharing. It won't be necessary here to do it that way, and probably not advisable. |
It's also possible to use SVG as the src attribute of an img tag: This might be prevented if we don't include ImageMetadata, right? I'd include it, to avoid a breaking change. |
|
Do it and we'll discuss on the PR, it will be simpler than here |
|
@florian-lefebvre here it is: withastro/astro#13651 |
19 participants
Summary
This RFC proposes adding native support for importing and rendering
.svgfiles as Astro components, with optimized rendering techniques to minimize performance impact. It aims to allow.svgfiles to be treated as components that accept props and be optimized for repeated use on a page using<symbol>and<use>elements.An SVG can be imported directly into an Astro component and used as a component that will only embed itself once.
Results in:
Links