facebook / docusaurus

Easy to maintain open source documentation websites.
https://docusaurus.io
MIT License
55.87k stars 8.37k forks source link

chore: tailor svgo config for inline svgs #10205

Closed SethFalco closed 3 months ago

SethFalco commented 3 months ago

Pre-flight checklist

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:

Test Plan

Before:

After:

Test links

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

Related issues/PRs

netlify[bot] commented 3 months ago

[V2]

Built without sensitive environment variables

Name Link
Latest commit 2bb9634e6df3f187bd3e9cd16674d40d6ad97ea3
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.

github-actions[bot] commented 3 months ago

⚑️ 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 commented 3 months ago

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

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

SethFalco commented 3 months ago

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!

techbridgedev commented 3 months ago

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.