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

chore: tailor svgo config for inline svgs #10205

Closed
wants to merge 2 commits into from

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Jun 9, 2024

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • N/A If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • N/A If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Users have encountered issues when multiple SVGs are inlined in the same page, which is a very common scenario, SVGO's cleanupIds plugin can introduce conflicting IDs after optimization.

Even without cleanupIds, SVGs are typically maintained separately and may happen to have conflicting IDs, which can be problematic when they use CSS selectors. I have checked to see if there's anything we could do better in SVGO, but nothing comes to mind without introducing state which doesn't seem worthwhile.

Imo, the only lasting solution to this is for tools that foresee this scenario to enable prefixIds. It's worth noting, SVGR actually enables prefixIds by default, but Docusaurus incidentally disables it when disabling removeViewBox and removeTitle.

I configured prefixIds in this PR to use a custom delimiter and prefix:

  • delim - because there's no need for it.
  • prefix - this part of your webpack configuration only applies to files ending with .svg, so there's no benefit to including that suffix in the prefix.

Test Plan

Before:

After:

Test links

Deploy preview: https://deploy-preview-10205--docusaurus-2.netlify.app/tests/docs/tests/svgs/

Related issues/PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 9, 2024
Copy link

netlify bot commented Jun 9, 2024

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 2bb9634
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/666622b160278e0008179ed9
😎 Deploy Preview https://deploy-preview-10205--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Jun 9, 2024

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 65 🟢 98 🟢 100 🟢 100 🟠 88 Report
/docs/installation 🟠 68 🟢 96 🟢 100 🟢 100 🟠 88 Report
/docs/category/getting-started 🟠 75 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog 🟠 69 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 63 🟢 96 🟢 100 🟢 100 🟠 88 Report
/blog/tags/release 🟠 71 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/tags 🟠 76 🟢 100 🟢 100 🟢 90 🟠 88 Report

@SethFalco
Copy link
Contributor Author

On the side, it may be nice to enable other SVGO plugins. We have a few plugins that are disabled by default, but perfectly acceptable to use for inlined vectors.

Reference: https://github.com/svg/svgo.dev/blob/main/src/plugins/configure-svgo.js#L26

  • mergePaths#noSpaceAfterFlags can be enabled, all modern browsers support this fine, it's just some vector editors like Illustrator/Affinity Design that don't.
  • removeXlink and removeXMLNS, browsers ignore namespaces, so it's perfectly safe to throw these away.

Have any thoughts on turning these on (in a separate PR)?

@SethFalco
Copy link
Contributor Author

SethFalco commented Jun 9, 2024

Ahh, sorry. I can see a PR like this was already submitted and rejected.

I'll shelve this, and later look into a good way to configure SVGO from Docusaurus. SVGO does have an svgo.config.js file, so we can probably check for it by default, or introduce a config somewhere where a developer can tell Docusaurus which file to read the config from.

If this PR catches your eye, I'd appreciate feedback on the comment above still!

@SethFalco SethFalco closed this Jun 9, 2024
@techbridgedev
Copy link

techbridgedev commented Jun 10, 2024

I support adding some method for configuring SVGO. I ran into the duplicate IDs problem referenced in issue 8297. Every time I add a new SVG to a page where IDs are already in use I have to go back and update empty IDs and refactor my stylesheet.

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

Successfully merging this pull request may close these issues.

Multiple inline SVGs on a page can clash
3 participants