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

Inconsistent viewBox attribute in <svg/> elements #249

Open
delilahw opened this issue Nov 7, 2024 · 7 comments
Open

Inconsistent viewBox attribute in <svg/> elements #249

delilahw opened this issue Nov 7, 2024 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@delilahw
Copy link

delilahw commented Nov 7, 2024

What version of astro-icon are you using?

v1.1.2

Astro Info

Astro                    v4.16.10
Node                     v18.20.3
System                   Linux (x64)
Package Manager          unknown
Output                   static
Adapter                  none
Integrations             astro-icon

If this issue only occurs in one browser, which browser is a problem?

No response


Describe the Bug

Hi! Thank you all for your work on this library, it's been super helpful in optimising icons and bringing them to my Astro site. 😇

In #246, the viewBox attribute was moved from <svg/> to <symbol/>. However, for non-inline icons, it appears that this only takes effect on the first occurrence of <Icon/>. Subsequent instances of <Icon/> retain their viewBox attribute, which is inconsistent behaviour.

That is,

{/* Put both of these `<Icon/>` instances, one after another, in the same Astro file: */}

{/* This renders `<svg/>` _without_ a `viewBox` attribute */}
<Icon name="square"/>

{/* This renders `<svg/>` _with_ a `viewBox` attribute */}
<Icon name="square"/>

I suspect the cause to be: includeSymbol: bool determines whether to remove the viewBox attribute. According to the below assignment, it will be false in subsequent usages of <Icon/> because i > 0.

const includeSymbol = !inline && i === 0;

const { viewBox } = normalizedProps;
if (includeSymbol) {
delete normalizedProps.viewBox;
}
---
<svg {...normalizedProps} data-icon={name}>


What's the expected result?

The behaviour should be consistent. A consumer should be able to specify the same markup (<Icon name="square"/>) and expect them to render <svg/> that behaves the same — it should either omit the viewBox attribute across all instances, or include them across all instances.

Another suggestion is to add some sort of attribute/config to let users decide the viewBox behaviour.


Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-u6hdxa?file=src%2Fpages%2Findex.astro

@alxndrsn
Copy link

I think I'm experiencing this - the first <Icon> on a page looks fine, but subsequent instances are invisible unless viewBox is manually removed from the generated <svg> elements in the page source.

@delilahw
Copy link
Author

@alxndrsn My current workaround is just pinning it to v1.1.1 for now. Hopefully someone has time to triage this soon =)

@alxndrsn
Copy link

alxndrsn commented Nov 12, 2024

@delilahw thanks 🙂

I replaced it with a custom component:

---
import icnA from '../icons/a.svg?raw';
import icnB from '../icons/b.svg?raw';
...

export function htmlFrom(iconName) {
  switch(iconName) { 
    case 'a': return icnA;
    case 'b': return icnB;
    ...
    default: return '<div>missing icon</div>';
  }
}

const { src } = Astro.props;
  
const iconHtml = htmlFrom(src);
---
<Fragment set:html={iconHtml /*eslint-disable-line astro/no-set-html-directive*/}/>

A bit annoying as every icon has to be added to the switch statement, but it gives direct control over what actually happens to the generated markup.

@stramel
Copy link
Collaborator

stramel commented Nov 23, 2024

Apologies! I'll get this fixed soon! This stemmed from #212

@stramel stramel added the bug Something isn't working label Nov 23, 2024
@stramel stramel self-assigned this Nov 23, 2024
@stramel
Copy link
Collaborator

stramel commented Nov 23, 2024

Investigation

So looking into this further. This is the current functionality in [email protected].

Why this happens

The <symbol> element does not define any intrinsic size—it relies on the viewBox for coordinate scaling but leaves sizing to be explicitly defined by the <svg> tag using the <use> element.

Options to Address It

Option 1: Specify both width and height or size (Current)

Explicitly setting the size on the icon will resolve the lack of sizing.

Option 2: Use CSS

Instead of hardcoding the width and height, you can define them via

<svg>
  <symbol id="icon-star" viewBox="0 0 24 24">
    <path d="M12 2l3.09 6.26L22 9.27l-5 4.87L18.18 22 12 18.27 5.82 22 7 14.14l-5-4.87 6.91-1.01L12 2z" />
  </symbol>
  <use href="#icon-star"></use>
</svg>

<svg class="icon">
  <use href="#icon-star"></use>
</svg>

<style>
  .icon {
    width: 50px;
    height: auto; /* Allows aspect ratio to be maintained */
  }
</style>

In this case:

CSS defines the dimensions, and you can use height: auto to let the aspect ratio control the scaling.

Option 3: Move the viewBox onto <svg>

You can define a viewBox directly on the <svg> element that uses <use>. This approach allows the browser to calculate dimensions based on the viewBox's aspect ratio, even if only one of width or height is specified:

<svg>
  <symbol id="icon-star" viewBox="0 0 24 24">
    <path d="M12 2l3.09 6.26L22 9.27l-5 4.87L18.18 22 12 18.27 5.82 22 7 14.14l-5-4.87 6.91-1.01L12 2z" />
  </symbol>
  <use href="#icon-star"></use>
</svg>

<svg viewBox="0 0 24 24" width="50">
  <use href="#icon-star"></use>
</svg>

Here:

The viewBox on <svg> takes precedence, so the width of 50 ensures proper scaling, and the height is calculated automatically based on the viewBox's aspect ratio.

Option 4: Use a Default Dimension on the <symbol>

Add default width and height attributes to the <symbol> itself. These will be inherited by <use> instances unless overridden.

Summary

We have a few options for direction, we can hopefully find the way that best fits everyone's needs. I will discuss with @natemoo-re and find out the best course of action. This will also impact the experimental Astro SVG Components on v5 beta.

@stramel
Copy link
Collaborator

stramel commented Nov 24, 2024

@delilahw @alxndrsn This library is going to be shifting to covering only iconify as we've merged the handling of SVG files primarily to Astro. Would love for you to test it out and provide some initial feedback!

Please try upgrading to astro@^5.0.0-beta.11

You can read the docs about it here: https://5-0-0-beta.docs.astro.build/en/reference/experimental-flags/svg/

You can provide feedback about it here: withastro/roadmap#1035

@fwextensions
Copy link

Glad I found this thread, as I was banging my head against the wall trying to figure out why the first instance of an svg icon looked right but subsequent ones didn't. @delilahw's suggestion of pinning to v1.1.1 fixes it, even with Astro v5.1.1.

@stramel keep in mind that the viewBox attribute isn't just about sizing the icon but also positioning it within the bounding box, which is not something you can do with CSS (AFAIK? I'm assuming left and top CSS styles wouldn't affect the svg viewBox).

In my case, I was using an icon that was flush up against the viewBox boundary, and I wanted to add some padding around it in certain cases but not others. I can't remember exactly why, but it seemed only possible (or just easier) to do with the viewBox than trying to add the padding with CSS. Worked great until I blindly updated astro-icon along with other deps when updating to Astro v5.

fwextensions added a commit to sfbrigade/sfbrigade.github.io that referenced this issue Dec 28, 2024
How the viewBox attribute is handled changed after v1.1.1, which caused the default icons used for blog posts with no photos to not look right in the previous push.
More info: natemoo-re/astro-icon#249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants