Skip to content

Conversation

tylersticka
Copy link
Member

@tylersticka tylersticka commented Oct 23, 2020

Overview

I started making an unrelated change to a pattern, but on every save I saw errors about SVG loading in the Terminal. These appear to be uncaught issues with changes from #862, which caused the icon listing to fail (which explains its disappearance from our deploy).

I tried a few different solutions, but there were three points of Webpack configuration around .svg files and whenever I fixed one a different one would have issues.

I looked back at the history of why there was so much SVG configuration, and realized it was all due to #503, specifically the need to convert SVG files to React components for the sole purpose of documentation.

Since that's the only context where we'd want to do that, I decided to use inline loaders within that file instead. While that's not a great practice for an entire project, it keeps this complexity confined to the single file that needs it.

Screenshots

Screen Shot 2020-10-23 at 10 54 52 AM

Testing

On the deploy preview...

  1. Observe that the icon listing is displayed correctly.
  2. Confirm that Twig inlined SVGs haven't been affected in examples like this.
  3. Confirm that CSS inlined SVGs haven't been affected by inspecting elements like this.
  4. Confirm that SVGs in prototypes like this haven't been affected.

/CC @spaceninja @calebeby

@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2020

⚠️ No Changeset found

Latest commit: 82437eb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

// @see https://webpack.js.org/guides/dependency-management/#context-module-api
const iconDir = require.context('../../assets/icons', false, /\.svg$/);
const iconDir = require.context(
'!!@svgr/webpack!../../assets/icons',
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment explaining this svgr syntax, or linking to some docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, this is more webpack syntax than svgr but I've linked to docs for both now.

Copy link
Member

@spaceninja spaceninja left a comment

Choose a reason for hiding this comment

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

Sounds like a great idea to me, and I'm very much in favor of reducing the complexity of our app. I'll leave final approval to @calebeby since this touches webpack, but it LGTM.

@tylersticka
Copy link
Member Author

FYI, no need for anyone to review till I've marked as "ready for review" and assigned a reviewer. Was waiting till the deploy preview finished so I could make the testing instructions easier.

@tylersticka tylersticka requested review from calebeby and a team October 23, 2020 18:18
@tylersticka tylersticka marked this pull request as ready for review October 23, 2020 18:18
@tylersticka tylersticka requested a review from calebeby October 23, 2020 18:45
Copy link
Member

@calebeby calebeby left a comment

Choose a reason for hiding this comment

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

🎉 Thanks for catching that!

@tylersticka tylersticka merged commit e85971b into v-next Oct 23, 2020
@tylersticka tylersticka deleted the fix/svg-load-issues branch October 23, 2020 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants