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

fix: cross domain SVG sprites #16244

Merged
merged 6 commits into from
Aug 16, 2024
Merged

fix: cross domain SVG sprites #16244

merged 6 commits into from
Aug 16, 2024

Conversation

zomars
Copy link
Member

@zomars zomars commented Aug 16, 2024

What does this PR do?

Follow up to #16135

Implemented icon sprite fetching to avoid CORS issues in Atoms, Embeds and Orgs subdomains.

image.png

What changed?

  • Added IconSprites component to render SVG sprites inline
  • Updated Icon component to use local references instead of external SVG files
  • Integrated react-inlinesvg library for efficient SVG handling
  • Modified app layouts and providers to include IconSprites and CacheProvider

How to test?

  1. Run the application and navigate through different pages
  2. Verify that icons are displayed correctly across the site
  3. Inspect the page source to confirm the presence of inline SVG sprites
  4. Check network requests to ensure SVG icons are not being loaded individually

Why make this change?

This change optimizes icon loading by using SVG sprites, which reduces the number of HTTP requests and improves overall page load performance. By rendering icons inline and utilizing a cache provider, we enhance the efficiency of SVG handling across the application.

More info on the SVG CORS subject:

https://oreillymedia.github.io/Using_SVG/extras/ch10-cors.html

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have added a Docs issue here if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

@zomars zomars requested review from a team August 16, 2024 15:52
@keithwillcode keithwillcode added core area: core, team members only foundation labels Aug 16, 2024
@dosubot dosubot bot added ui area: UI, frontend, button, form, input 🐛 bug Something isn't working labels Aug 16, 2024
Copy link

vercel bot commented Aug 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Aug 16, 2024 4:16pm
calcom-web-canary ⬜️ Ignored (Inspect) Visit Preview Aug 16, 2024 4:16pm

export function IconSprites() {
return (
<SVG
src={`${process.env.NEXT_PUBLIC_WEBAPP_URL}/icons/sprite.svg?v=${process.env.NEXT_PUBLIC_CALCOM_VERSION}`}
Copy link
Member Author

Choose a reason for hiding this comment

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

This will help caching aggressively between releases

Copy link
Member Author

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Ready for review

Copy link

graphite-app bot commented Aug 16, 2024

Graphite Automations

"Add foundation team as reviewer" took an action on this PR • (08/16/24)

1 reviewer was added to this PR based on Keith Williams's automation.

@PeerRich PeerRich enabled auto-merge (squash) August 16, 2024 16:21
@PeerRich PeerRich merged commit 101fea2 into main Aug 16, 2024
36 of 37 checks passed
@PeerRich PeerRich deleted the svg-sprite-rewrite-tests branch August 16, 2024 16:33
Copy link
Contributor

E2E results are ready!

@Ryukemeister
Copy link
Contributor

Can confirm this works for atoms as well! @zomars YOU ARE A LEGEND! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working core area: core, team members only foundation ready-for-e2e ui area: UI, frontend, button, form, input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants