search

natemoo-re / astro-icon

Inline and sprite-based SVGs in Astro made easy!
https://astroicon.dev
Other
996 stars 57 forks source link

Allow control of second argument for `iconToSVG` #125

Open charlie-hadden opened 1 year ago

charlie-hadden commented 1 year ago

Hi there,

I'm trying out v1 - so far it's working well for me aside from one issue. I see that here, you call iconToSVG providing only one argument: https://github.com/natemoo-re/astro-icon/blob/e66e3adc8747f2b5750ae036957d7ae00a54b2ce/packages/core/components/Icon.astro#L102

This causes default width and height attributes to be set on the SVG, which is something which I don't want for my use case. Iconify does provide a way to avoid this using the second argument to iconToSVG, but there's no way to control that. An alternative solution would be to pass the width and height to the Icon component, but because of the typescript types for the props, I can't see a way to clear the attributes without having to work around a type error.

Aside from the dimensions, there are also some other parameters in the customization options which look useful. I'd love to see some way of controlling that parameter. Perhaps by exposing it as an optional prop on the Icon component, and maybe even setting defaults in the integration config, if that's possible.

Thanks!

stramel commented 12 months ago

Thank you for filing this issue! I can look into adding the a way to pass the additional parameter. Would it cover your use-case to allow unset or auto in the height/width properties?

charlie-hadden commented 12 months ago

Thanks for taking a look at this one. For my particular use case, passing unset in the height property would be sufficient, yes. I do think that the 1em height that gets set is probably a good default too, but I think that's a breaking change from the v0 version of astro-icon, so might be worth a note in the changelog.

stramel commented 11 months ago

@charlie-hadden Can you provide an icon that you're testing and what you would expect it to be? That will help me ensure I get a good resolution for this.

charlie-hadden commented 11 months ago

I've taken a bit more of a look into this, and set up a bit of a demo/writeup here https://github.com/charlie-hadden/astro-icon-reproduction/blob/unset-height/src/pages/index.astro. If you spin up that project locally then you should be able to see the behaviour I'm talking about.

I also poked around with the astro-icon source a little, and handling the transformation options (vFlip, hFlip, rotate) would require a little extra work than I initially thought. Because they modify the SVG, the caching for icon reuse would need to be updated as otherwise the transformations applied to the icon on its first use would also apply to all later uses of the same icon. I do think that would be a nice feature to have though.

If the decision is to only support the sizing use case, then I think expanding the type for the sizing props to number | "auto" would be enough.

Do check out the demo I put together though - there are some bits which were a little easier to explain with an example. If more complete control of those options and sizing is something which this project is interested in then I'd be happy to put together a PR for it.

fflaten commented 6 months ago

If the decision is to only support the sizing use case, then I think expanding the type for the sizing props to number | "auto" would be enough.

Related #163

natemoo-re commented 6 months ago

Sorry about that PR @charlie-hadden! If you're still interested in this, I'm definitely on board with supporting this. Maybe under a transform object prop?

charlie-hadden commented 6 months ago

@natemoo-re no worries! I can take a look at creating a new PR after the holidays if nobody gets to it before me.