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

brick: Use data URI SVG #23

Merged
merged 5 commits into from
Sep 3, 2020

Conversation

jrchamp
Copy link
Contributor

@jrchamp jrchamp commented Aug 25, 2020

Fixes #17
Fixes #26

It looks like main.js doesn't exist any more, so if accepted, this will reduce both the number of total network requests and bytes.

I took the .eps from brand.ncsu.edu, passed it through inkscape to get a plain SVG, crunched it down with SVGOMG precision 3 and fixed the red color to #c00 (3384 bytes uncompressed when base64 encoded).

I didn't rebuild the dist file as part of this, so that will need to be added to this before merging.

Please let me know what you think!

@apmatthews
Copy link
Contributor

Looks good to me. I pulled it down and tested locally. I think it makes sense to add the dist file and then look at merging.

I did end up removing main.js in c35fd5e, for reference.

Something else to consider: https://css-tricks.com/probably-dont-base64-svg/

The other SVGs on this project are base64 so I don't think we have to do the brick differently, but I've used that article's method for the past couple of years with good results.

@jrchamp
Copy link
Contributor Author

jrchamp commented Aug 26, 2020

Hi @apmatthews

I used the URL encoding process as described in the article you mentioned on each of the SVGs. The new scss file is 17.3% smaller than the original (even after adding the brick SVG as a data URI).

@jrchamp jrchamp force-pushed the feature/brick-via-data-uri branch from 26059d4 to 2490799 Compare August 27, 2020 04:44
@apmatthews
Copy link
Contributor

@jrchamp Awesome! Looks like you were able to get it to build too? I was having some trouble building for production (npm run production). I think it's probably due to an old bug in SVGO. My guess is this wasn't an issue before because there wasn't any true SVG markup in the source, but the new URL encoded SVGs are probably being picked up by SVGO. #26 might be the answer, but I'm curious if you ran into any issues in 2490799.

@jrchamp
Copy link
Contributor Author

jrchamp commented Aug 27, 2020

I had to tell postcss-svgo to leave it alone since it was already optimized to get it to build. #26 solved the issue for me.

@jrchamp
Copy link
Contributor Author

jrchamp commented Aug 27, 2020

Do you want me to add the build system update that worked for me to this pull request?

@apmatthews
Copy link
Contributor

Yes, please 🙂

@jrchamp
Copy link
Contributor Author

jrchamp commented Aug 27, 2020

Done.

@jrchamp
Copy link
Contributor Author

jrchamp commented Sep 1, 2020

It's all building cleanly for me. sass-loader should be current now too. Please let me know if you have any questions.

@apmatthews
Copy link
Contributor

I'm able to build now, but seeing this:

Screen Shot 2020-09-01 at 2 56 07 PM

The issue is that whitespace isn't being stripped out of the HTML template during the production build. I think that's what this line is supposed to do, but maybe it's not respected in the latest version of the build packages?

@jrchamp
Copy link
Contributor Author

jrchamp commented Sep 1, 2020

Hmm, I think they moved some of the "options" to "sassOptions".

@apmatthews
Copy link
Contributor

I just added the dist folder to .gitignore to make PRs a little simpler and cleaner. Would you mind merging that in from master and excluding the dist updates from this PR?

@jrchamp jrchamp force-pushed the feature/brick-via-data-uri branch from d1c7465 to 376f8eb Compare September 1, 2020 19:32
@jrchamp
Copy link
Contributor Author

jrchamp commented Sep 1, 2020

Done. I rebased the changes so that it doesn't have the dist file flipping back and forth. It looks like minimize: true has removed the extra whitespace characters from the html-loader.

@apmatthews apmatthews merged commit 7c5113b into ncstate:master Sep 3, 2020
apmatthews pushed a commit that referenced this pull request Sep 3, 2020
@apmatthews
Copy link
Contributor

@jrchamp Thanks for the contribution and for rolling with all the build updates. I've dropped it on the CDN. I'm not entirely sure how long it takes to move through all the pipes, but we should see this update in production sometime today. Thanks again!

@jrchamp jrchamp deleted the feature/brick-via-data-uri branch September 3, 2020 22:57
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.

Update build system Wishlist: Reduce HTTP Requests to 1
2 participants