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

Support generated IDs to avoid collisions in HTML #27

Closed
sdvg opened this issue Jan 7, 2019 · 2 comments
Closed

Support generated IDs to avoid collisions in HTML #27

sdvg opened this issue Jan 7, 2019 · 2 comments

Comments

@sdvg
Copy link
Contributor

@sdvg sdvg commented Jan 7, 2019

Consider this example: https://codepen.io/anon/pen/bOMQjK

There is a symbol with the ID email (email.svg) and an input field with the same ID and a connected label (<label for="email">).

Displaying the SVG works, but the label breaks in this example.

This could be solved by adding an option which would allow a custom function to generated the ID. For most projects it should be sufficient to provide a prefix like icon- to solve most conflicts. For more complex projects providing a generated UUID should also work.

I would be happy to create a PR if you like the idea :)

@DHedgecock
Copy link
Member

@DHedgecock DHedgecock commented Jan 7, 2019

👍 Awesome enhancement.

I'm thinking the loader should accept a symbolId function which will be passed the complete filepath, for maximum flexibility determining what the id for each symbol in the sprite should be.

For your example the user config could be something like:

{
  loader: 'svg-symbol-sprite-loader',
  options: {
    // Use filename as symbol id prefixed with icon- to prevent DOM id collisions
    symbolId: filePath => `icon-${path.basename(filePath, '.svg')}`
  }
}

For repo context, I think the option just needs to be passed from loader.js into sprite-store.js at the addSVG call.

A PR would be great, thanks 🎉 (I won't be online for 8 days after midweek so if you don't hear back please be patient)

Thanks again for the suggestion 👍

DHedgecock added a commit that referenced this issue Jan 22, 2019
Thanks to @sdvg
@DHedgecock
Copy link
Member

@DHedgecock DHedgecock commented Jan 22, 2019

Thanks for your patience while I was on vacation @sdvg

This should be available in the latest release now 🎉

@DHedgecock DHedgecock closed this Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants