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

STAR-110 Remove all IDs from SVG files #405

Merged
merged 3 commits into from
Jan 30, 2020

Conversation

DanilAgafonov
Copy link

@DanilAgafonov DanilAgafonov commented Jan 24, 2020

Purpose 🚀

If we insert any icon to page more than one time, then we get ID collision. This PR eliminates this issue.

Updates 📦

  • MINOR (backward compatible) change to @paprika/icon

Storybook 📕

http://storybooks.highbond-s3.com/paprika/STAR-110-duplicate-svg-ids

References 🔗

https://aclgrc.atlassian.net/browse/STAR-110

@DanilAgafonov
Copy link
Author

I was looking for automated solution. But found only this issue: svg/svgo#563

The only option for now - remove redundant <defs>/<use> manually.

@@ -14,7 +14,7 @@
"access": "public"
},
"scripts": {
"svgr": "npx @svgr/cli -d . src/svg --out-dir src/"
"svgr": "svgr -d . src/svg --out-dir src/"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason of this change: npx use latest version of @svgr/cli from npmjs.com. It means that version lock ("@svgr/cli": "^4.1.0") will be ignored. After this change Yarn will run ./node_modules/.bin/svgr.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch 💯

@DanilAgafonov DanilAgafonov merged commit 08238e0 into master Jan 30, 2020
@DanilAgafonov DanilAgafonov deleted the STAR-110-duplicate-svg-ids branch January 30, 2020 09:38
@strarsis
Copy link

strarsis commented Sep 4, 2020

@DanilAgafonov: PR ready in svgo, see svg/svgo#563 (comment).
You can try out the new svgo dereferenceUses plugin.

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

Successfully merging this pull request may close these issues.

3 participants