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

Consider running iconify's replaceIDs function on SVGs #127

Open
charlie-hadden opened this issue Jul 11, 2023 · 2 comments
Open

Consider running iconify's replaceIDs function on SVGs #127

charlie-hadden opened this issue Jul 11, 2023 · 2 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@charlie-hadden
Copy link

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

const normalizedBody = renderData.body;

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 stramel added the enhancement New feature or request label Jul 14, 2023
@stramel stramel self-assigned this Jul 14, 2023
@stramel stramel added this to the v1 milestone Jul 14, 2023
@stramel
Copy link
Collaborator

stramel commented Jul 14, 2023

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
Copy link
Author

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 #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants