-
Notifications
You must be signed in to change notification settings - Fork 31
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
[RFC Stage 3]: Built-in SVG Components #1035
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @stramel for the hard work! I left some preliminary feedback :)
proposals/0052-svg-components.md
Outdated
<svg width="48" height="48"> | ||
<!-- SVG Content --> | ||
</svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should go in more detail, and explain how the component will behave with width
and height
. For example, given these two cases where we have the same props in a different order, what is the end result?
<>
<Logo size={50} width={20} />
<Logo width={20} size={50} />
</>
I believe it's also fine to consider an error if the two props are incompatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree this deserves more detail around a somewhat tricky prop. I left this as an outstanding question since I wasn't sure the best way to handle the overlap of height/width props & attributes, the size prop and intrinsic size data coming from the image metadata. https://github.com/withastro/roadmap/pull/1035/files#diff-ab3122e95078e177eab4f364380c44e582e73c496b98809d7d5e966f9d09e1f1R247
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever logic we decide is best, it is probably worth adding a warning around it if we decide not to apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
- **Framework-Specific Docs:** Documentation will also need to address framework-specific usage. This will cover the limitations of the `.svg` imports in popular frameworks like React and Vue, ensuring smooth adoption for developers who are building Astro projects using these frameworks. | ||
- **Code Examples:** The core team can provide boilerplate examples and starter templates that showcase the optimized `.svg` handling. These templates could be adapted for common use-cases, such as building an icon library or optimizing assets for a marketing page. | ||
- **Experimental Flag:** Initially, this feature could be released under an experimental flag in `astro.config.mjs` to gather feedback from early adopters. If the feedback is positive, the feature could later be enabled by default in a future Astro release. | ||
- **Migration Path:** Since this feature is backwards-compatible, no migration will be required for existing projects. Developers can opt into the new functionality by updating their `.svg` imports, but their projects will continue working without any changes if they choose not to adopt the new behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is backwards compatible? Doesn't importing a .svg
file today return the path to the file? What does it do currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we're able to make this backwards compatible by assigning the metadata to the component function. (https://github.com/withastro/astro/pull/12067/files#diff-2036fac95cff79ff0745b8b3f58e7b3131492cb4671271d489f423df0226bfc8R53)
importing an .svg
file today returns a metadata object:
import { src, width, height } from '../../icons/logo.svg';
You will still be able to do this in the new implementation.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
proposals/0052-svg-components.md
Outdated
### 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
title
attribute gets confusingly converted into a<title>
element. This is surprising behavior.- The
title
attribute 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's a nuanced topic, that's why I'm attempting to have a nuanced conversation. Calling something "code-smell" and linking to hyperbolic articles is not displaying nuance. Let's please do so and stick to observable facts and specifics.
Your test case also specifically only focused on SVGs inside a , so it's not even testing the accessibility of the SVG itself, but rather that of the button. This distinction is important because the button role flattens any semantic information inside it into a single string which gets used for labelling.
This is the use-case we're discussing. Inside of <button>
or inside of a <a>
, both which have roles and will announce (tested). It's ok for a prop to exist to aid in one use-case without it being as useful to others.
If there are other AT to test this with please let me know what those are and I'll do so.
I'm still not sure what is going on with Safari here and need to dig into it deeper. It's also not announcing with role="img"
so there might be something else wrong with my page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following ("pattern 8" mentioned above) should work in VoiceOver+Safari and everywhere else:
<button>
<svg role="img" aria-labelledby="unique-id">
<title id="unique-id">Favorite</title>
</svg>
</button>
There are lots of other assistive technologies, like screen magnifiers, refreshable braille displays, switch controls, sip-and-puff devices. You can take a look at how people with disabilities use the web to better understand the assistive technologies they rely on. An easy one to test with (and one that's directly relevant here) is speech recognition software.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
proposals/0052-svg-components.md
Outdated
### 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
### Sizing | ||
|
||
To simplify the process of setting the dimensions of SVG components, this proposal introduces a `size` prop that developers can use to uniformly scale the width and height of an SVG. This prop provides a convenient shorthand for situations where both width and height need to be set to the same value, offering a more concise API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there's enough benefit to having this size
prop. It adds some complexity, while saving users only a few characters of typing at most.
Three downsides I can think of:
- The types now diverge from
SVGAttributes
. - Unexpected results when the SVG isn't intended to have a 1:1 aspect-ratio.
- Additional API for users to discover and learn.
I think a framework wrapper over SVG should stay as close to <svg>
as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the feedback! This was a carryover from astro-icon. We primarily had the use-case of square icons so this was an extremely useful. I'm open to removing it if we feel that it isn't super useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ties back to an unresolved question as well.
https://github.com/withastro/roadmap/pull/1035/files#diff-ab3122e95078e177eab4f364380c44e582e73c496b98809d7d5e966f9d09e1f1R328
This is working great in the latest release, as of time of comment, 5.0.4. Here again to kindly request support for the following import syntax as possible... <SVG src={import('@icons/houston.svg')} /> Scrolling up and down larger files to manage import paths is a chore. Choice of the option would be nice. I'm also wondering if it's possible to roll your own way to do this without any additional plugins but I don't see a way currently. Thank you. |
---
const Svg = Astro.props.src;
---
<Svg /> I wonder if this works? |
- **Limited Use Cases:** The performance benefits of the Sprite approach may be minimal for projects that don't heavily rely on SVGs. For smaller projects or those with limited SVG usage, the optimization might introduce unnecessary complexity without providing significant gains in performance. | ||
- **Edge Cases with Advanced SVG Features:** Some advanced features of SVGs, like `<filter>` or `<mask>`, _may_ have issues when used within a `<symbol>` and referenced by `<use>`. These features will need thorough testing to ensure they function properly in the optimized model. | ||
|
||
# Alternatives |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not discussed in the alternative is having a <Svg />
component, similar to how we have an <Image />
component rather than having the image import become a component. Going to bring this up with others on core we might remember better than me why we made that choice before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got prior art in https://github.com/jasikpark/astro-svg-loader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also consider to make it work with dynamic paths. Something like ./logos/${icon}.svg
is already working with the above suggested custom component. But more dynamic imports like ${BASE_DIR}${logo}.svg
works in development but fail to bundle correctly due to vite-rollup static analysis limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to adding support for dynamic imports
The core design of this proposal allows `.svg` files to be imported and used as components in Astro. This means developers can import `.svg` files into their Astro components and render them directly as if they were Astro components. The goal is to maintain ease of use while automating performance optimizations under the hood. | ||
|
||
**Example:** | ||
|
||
```astro | ||
--- | ||
import Logo from '../assets/logo.svg'; | ||
--- | ||
<Logo /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have svg work like the image component, because I lot of imports would be a waste of time and take website resources. Where you import the component and then provide the svgs inside of the component.
Summary
This RFC proposes adding native support for importing and rendering
.svg
files as Astro components, with optimized rendering techniques to minimize performance impact. It aims to allow.svg
files 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