natemoo-re / astro-icon

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

Consider running iconify's `replaceIDs` function on SVGs #127

Open charlie-hadden opened 11 months ago

charlie-hadden commented 11 months ago

Hi again and sorry for the wave of issues here. This is another one for v1.

Some of the SVGs I'm using reference elements by ID. This works fine when there's a single SVG on the page, but with multiples it's possible for the IDs to collide causing issues with things such as masks. Iconify provides a function which resolves this for me: https://iconify.design/docs/icon-components/svg-framework/replace-ids.html

I currently have a pnpm patch which adds the import and replaces https://github.com/natemoo-re/astro-icon/blob/e66e3adc8747f2b5750ae036957d7ae00a54b2ce/packages/core/components/Icon.astro#L104 with

const normalizedBody = replaceIDs(renderData.body);

This is enough to fix the issue for me by generating unique IDs for each. I'd be happy to submit a PR for this unless it's more convenient for somebody else to do so, but before I do, is there any preference on what prefix to use? Using replaceIDs as above generates ID's like so:

image

Thanks!

stramel commented 11 months ago

How are you referencing these ids if they're random? You can also use the prefixIds plugin to add prefixes. Something like:

  plugins: [
    'cleanupIds',
    {
      name: 'prefixIds',
      params: {
        prefix() {
        this.counter = this.counter || 0;

        return `id-${this.counter++}`;
      }
      }
    }
  ]

I'm going to consider adding an option to enable this. I think it could cause unwanted issues for people looking to directly reference the ids.

charlie-hadden commented 11 months ago

I've set up another demo to explain what I mean: https://github.com/charlie-hadden/astro-icon-reproduction/tree/replace-ids

If you give it a try then please use pnpm as there's a patch for astro-icon there to pass the svgo options through (as per https://github.com/natemoo-re/astro-icon/pull/129).

If you run that project then you'll see there are two SVGs displayed. In astro.config.mjs, if you uncomment the cleanupIds plugin then you'll see the issue. There's some other comments there with examples of prefixIds and why, as far as I can tell at least, that won't work for this use case. I'm not an expert with svgo though, so it's possible there's something I'm missing.

I then have another branch which has a pnpm patch to add the replaceIDs call, and you can see the difference in results: https://github.com/charlie-hadden/astro-icon-reproduction/tree/replace-ids-patched

This issue was also quite confusing for me when I first ran across it, because at that point I didn't have any custom svgo config at all - the default preset is enough to trigger the issue with these SVGs.

Hopefully that explains what I mean a little better but let me know if there's anything you'd like me to expand on.