poppa / sveltekit-svg

SvelteKit plugin that makes it possible to import SVG files as Svelte components, inline SVG code or urls
MIT License
235 stars 23 forks source link

Pass attributes to Svelte components #5

Closed vabatta closed 2 years ago

vabatta commented 3 years ago

Hi,

I was wondering if it would be possible to pass attributes to the SVG when loaded as Svelte components. For example:

<script>
import FacebookLogo from 'line-awesome/svg/facebook-f.svg';
</script>

<FacebookLogo class="my-class" />
poppa commented 3 years ago

I will take a look at it in the coming days ๐Ÿ‘

poppa commented 3 years ago

@vabatta I don't know if this is such a good idea. I think it will be difficult to manage this in an efficient way since you need to parse all attributes in the SVG to merge the values if the attribute passed from the Svelte component already exists in the SVG node. Take your class attribute for example: It's not unthinkable that an SVG already has a class attribute.

What I usually do is wrapping the SVG so I can style it from the parent node:

<div class="svg-wrapper">
  <MySvg />
</div>

<style>
.svg-wrapper {
  :global(svg) {
    // svg style
  }
}
</style>

Let me know if you have a good argument for forwarding attributes to the SVG and I will reconsider ๐Ÿ‘

vabatta commented 3 years ago

I would leave the job of making sure class and attributes don't clash to the developer, honestly. For example, you as a developer might know that the svg being used in a certain context have a fill attribute which makes useless a class that changes the color (unless fill="currentColor"). Also, when it comes to class merging is more about the stylesheet generation order that makes the classes get applied to the SVG; which again is the dev job to make sure they are the right order for what you need (I am thinking of Tailwind that does the job for you already :) ). Finally, for merging prop classes and SVG ones, I would use a tiny runtime library (228B!) to not have other implementation laying around.

With this said, I would definitely add the possibility and then the dev needs to make sure he's not breaking things :)

aradalvand commented 3 years ago

Hi @poppa, I was just palying around with this plugin and I expected this to have already been implemented honestly.

For instance, vite-plugin-svelte-svg and rollup-plugin-svelte-svg have this feature.

It's very straightforward to implement, all you have to do is add a {...props} to the end of the opening tag before passing the string to the Svelte compiler.

Here's how "vite-plugin-svelte-svg" does it: see Here's how "rollup-plugin-svelte-svg" does it: see

As simple as it gets.

aradalvand commented 3 years ago

since you need to parse all attributes in the SVG to merge the values if the attribute passed from the Svelte component already exists

You don't, just put {...props} after all the attributes and the user-defined attributes will take precedence, Svelte already takes care of stuff like that.

poppa commented 3 years ago

Hi @AradAral. You are more than welcome to create a PR ๐Ÿ‘

Tropix126 commented 2 years ago

since you need to parse all attributes in the SVG to merge the values if the attribute passed from the Svelte component already exists

You don't, just put {...props} after all the attributes and the user-defined attributes will take precedence, Svelte already takes care of stuff like that.

{...$$restProps}* in this case

aradalvand commented 2 years ago

@Tropix126 Well, you use $$restProps when your component itself has props, in which case โ€$$restPropsโ€ would contain the "rest of the props" passed to the component, as the name implies. In this case, where the component doesn't have any props, both $$props and $$restProps would be the same.

aradalvand commented 2 years ago

Thank you @poppa for implementing this! ๐Ÿ’™