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

add static svg sprite sheet file #1763

Merged
merged 4 commits into from Nov 3, 2023
Merged

add static svg sprite sheet file #1763

merged 4 commits into from Nov 3, 2023

Conversation

dstaley
Copy link
Contributor

@dstaley dstaley commented Nov 1, 2023

πŸ“Œ Summary

This PR adds support for a plain SVG sprite sheet (in addition to the existing JavaScript module version).

πŸ› οΈ Detailed description

In order to support using the SVG sprite sheet in our Next.js websites without shipping the sprite sheet as several hundred kilobytes of JavaScript, we'd like to simply reference an SVG file. This PR outputs the generated SVG sprite sheet to an SVG file in addition to the existing JS module. Two attributes were added to the root <svg> element for maximum compatibility:

  1. xmlns="http://www.w3.org/2000/svg" Some browsers will not correctly render remote <use> references unless the remote document has the correct namespace.
  2. viewBox="0 0 0 0" Next.js expects there to be a viewBox attribute to calculate dimensions. While this isn't actually useful in this case, it does prevent Next.js from throwing an error about being unable to parse the file.

Neither of these changes should have any impact on existing usage, hence the minor version bump.
Β 

πŸ“Έ Screenshots

πŸ”— External links

Jira ticket: HDS-XXX
Figma file: [if it applies]


πŸ‘€ Component checklist

  • Percy was checked for any visual regression
  • A11y tests have been run locally (yarn test:a11y --filter="COMPONENT-NAME")
  • If documenting a new component, an acceptance test that includes the a11yAudit has been added
  • A changelog entry was added via Changesets if needed (see templates here)

πŸ’¬ Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Nov 1, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
hds-showcase βœ… Ready (Inspect) Visit Preview Nov 3, 2023 6:02pm
hds-website βœ… Ready (Inspect) Visit Preview Nov 3, 2023 6:02pm

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

@dstaley I've added a couple of suggestions (adding comments to the code for future reference) but apart from that... all good to go! πŸ‘

Thanks for the contribution! πŸ™

@didoo didoo requested a review from a team November 2, 2023 16:59
dstaley and others added 2 commits November 2, 2023 10:30
Co-authored-by: Cristiano Rastelli <cristiano.rastelli@hashicorp.com>
Co-authored-by: Cristiano Rastelli <cristiano.rastelli@hashicorp.com>
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

The xmlns and viewBox changes look good! Assuming you tested the svg-sprite.svg in your project I'm happy to get it in.

.changeset/mighty-singers-prove.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you for proposing something that works best for your use case.

Co-authored-by: Alex <alex-ju@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants