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

docs(assets): add note to inlining SVG through url() #15534

Merged
merged 5 commits into from
Mar 3, 2024

Conversation

chaejunlee
Copy link
Contributor

@chaejunlee chaejunlee commented Jan 8, 2024

Description

related #15378
related #15444

Since PR #14643, inlining SVG requires double quotes to correctly parsed on build.

There was no documentation around that, and there were 2 issues regarding it.

I have added a "note" with an example that was brought up in the issue.

TO-BE

Screenshot 2024-01-07 at 19 16 59

Additional context

Screenshot 2024-01-07 at 19 10 29

I wanted to put a styled-component example, however, eslint forced the double-quotes to convert back to single-quotes and the spacing between code blocks wasn't pretty. So, I erased the styled-component one.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Jan 8, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@chaejunlee chaejunlee changed the title docs(assets): docs(assets): add note to inlining SVG through url() Jan 8, 2024
@spalberg
Copy link

spalberg commented Jan 8, 2024

This just fixed my 2h bug hunting quest. Thanks!

docs/guide/assets.md Outdated Show resolved Hide resolved
@sapphi-red sapphi-red added the documentation Improvements or additions to documentation label Mar 1, 2024
Co-authored-by: 翠 / green <green@sapphi.red>
docs/guide/assets.md Outdated Show resolved Hide resolved
@patak-dev patak-dev merged commit 0c0aeae into vitejs:main Mar 3, 2024
10 checks passed
jeremywiebe added a commit to Khan/perseus that referenced this pull request Apr 1, 2024
## Summary:

Mark noticed that the new @phosphor icons were not showing up in Storybook on Github Pages (`khan.github.com/pereseus`). 

I tracked it down to a feature introduced in Vite 5 (vitejs/vite#15534), but it seems like it has bugs ([#15378](vitejs/vite#15378) and [#15444](vitejs/vite#15444)). 

I don't see how we could "quote" the URL as it's handed of to Wonder Blocks which seems to be handling the import correctly. 

So instead we'll just force Vite to never inline SVGs using Vite's `assetsInlineLimit` (https://vitejs.dev/config/build-options#build-assetsinlinelimit) option. 

Issue: LEMS-1887

## Test plan:

I haven't tested on GH Pages yet, but I did the following and verified that there are now svg files in the build output folder:

```sh
yarn build-storybook
ls storybook-static/assets
```

Author: jeremywiebe

Reviewers: benchristel, jeremywiebe, mark-fitzgerald

Required Reviewers:

Approved By: benchristel

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Extract i18n strings (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1136
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants