Experiment: load gridicons from external SVG file. #32172
Conversation
sgomes
added
[Type] Enhancement
[Status] Needs Review
|
Works in IE11. :) |
| onClick={ onClick } | ||
| { ...otherProps } | ||
| > | ||
| <use xlinkHref={ `/calypso/images/gridicons/sprite.svg#${ iconName }` } /> |
There was a problem hiding this comment.
do we need to cachebust this somehow?
There was a problem hiding this comment.
maybe use file-loader to get the url, which can bake the hash in?
There was a problem hiding this comment.
Yes, we'll need to have both a mechanism for keeping gridicons up-to-date from the gridicons repo, and a mechanism to keep users' copies up-to-date (with some form of cache-busting, yes).
I intentionally omitted any attempt at either of those in this PR, since I'm just looking for validation on the general approach, for now.
There was a problem hiding this comment.
I think the approach is good. How do you want to proceed with this?
There was a problem hiding this comment.
I think the approach is good. How do you want to proceed with this?
Here are my initial thoughts:
Work in gridicons repo:
- Modify build to include spritemap SVG as part of the gridicons NPM package.
- Modify build to include offset configuration as part of the gridicons NPM package (see the
icons-offset.jsfile in this PR) - For both of the above changes, we could keep the entry points as they are, so that existing consumers aren't affected. These would be extra files on the same package that would have to be required explicitly.
- Deploy new NPM package.
Work in Calypso repo:
- Increment the gridicons NPM package version to match the new release.
- Include the copying of the spritemap as part of the build process. Cache-bust spritemap based on the NPM package version, if that's somehow possible.
- Implement the rest of the approach as per this PR.
How does that sound?
There was a problem hiding this comment.
Sounds good. I think we have an earlier candidate for svg4everybody over in #32236. @griffbrad Do you need to coordinate that work at all?
There was a problem hiding this comment.
@griffbrad 's approach for the Material icons looks quite different, in that he's wrapping each icon in a React component, created at build-time through SVGR. That's the approach previously used in gridicons (SVG-in-JS), albeit with a simplified pipeline. It's a good approach if you're using an icon or two, but if you're using a large number of them (like we do in gridicons), the cost of wrapping SVG in JS starts taking a toll.
svg4everybody isn't needed in @griffbrad 's case, since as far as I can tell he's not using SVG <use> pointing to external resources, nor actually loading anything from SVG files at runtime.
There was a problem hiding this comment.
svg4everybody isn't needed in @griffbrad 's case, since as far as I can tell he's not using SVG pointing to external resources, nor actually loading anything from SVG files at runtime.
Right, I'd like to consolidate down to one method of loading external SVGs rather than both of these and I'd rather avoid transpiling svg to react.
There was a problem hiding this comment.
Gotcha. Feel free to add me as a reviewer in any SVG-loading PRs, then, and please feel free to reuse any of the code in this PR! I'll see if I can get to work on the gridicons side of things before continuing here in Calypso.
sgomes
force-pushed
the
try/gridicons-from-external-svg
branch
from
044df83 to
6df9a90
Compare
Gridicons are currently being wrapped in JSX and included as a separate JS bundle. This is suboptimal for a number of reasons, among which: - JS is byte-for-byte much more CPU-demanding than SVG, a concern on mobile devices. The fewer JS bytes the browser needs to handle, the better. - Encoding image data as JS invalidates any optimizations the browser may be able to otherwise perform, such as download prioritization. This PR experiments with an alternate approach: SVG external content. SVG data is kept in a static file, with each icon being assigned an ID. We then have a small component which simply references the correct ID in this external file, leaving it to the browser to fetch the data at an appropriate time during bootup. Since the SVG data is all kept in a single file, we strike a good balance between cacheability and number of requests, we avoid having large amounts of procedurally-generated JSX, and we delegate all image fetching to the browser, which can apply the usual optimizations as it sees fit. Potential caveats that appear to be handled correctly in this PR: - IE11 and older versions of modern browsers work correctly, through the svg4everybody polyfill (~1KB). The polyfill is not included in the evergreen bundle. - Icons are aligned correctly by copyign existing alignment logic. Some minor changes to existing CSS rules were needed. - A blank icon is shown when the icon name is unknown.
sgomes
force-pushed
the
try/gridicons-from-external-svg
branch
from
6df9a90 to
1cd2c0d
Compare
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints Legacy SCSS Stylesheet Sections Async-loaded Components Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
|
Not rebasing, as this PR is intended to be closed and replaced with a new PR after Automattic/gridicons#307 gets merged and a new NPM package for gridicons is deployed. |
|
This PR is superseded by #32763. |
matticbot
removed
the
[Status] Needs Review
Gridicons are currently being wrapped in JSX and included as a separate JS bundle. This is suboptimal for a number of reasons, among which:
This PR experiments with an alternate approach: SVG external content.
SVG data is kept in a static file, with each icon being assigned an ID.
We then have a small component which simply references the correct ID in this external file (using
<use>, a native SVG feature), leaving it to the browser to fetch the data at an appropriate time during bootup.Since the SVG data is all kept in a single file, we strike a good balance between cacheability and number of requests, we avoid having large amounts of procedurally-generated JSX, and we delegate all image fetching to the browser, which can apply the usual optimizations as it sees fit.
Potential caveats that were tested and appear to be handled correctly in this PR:
svg4everybodypolyfill (~1KB). The polyfill is explicitly excluded from the evergreen bundle, as it's not needed.Note: this PR is not intended to be merged as-is. I'm looking to validate the approach before moving forward. If it's approved, the work will likely need to be split between this repo and the
gridiconsrepo, with updates to thegridiconsNPM package.Changes proposed in this Pull Request
AsyncGridiconscomponentExternalGridiconscomponentgridiconsrepo and use inExternalGridiconsstatic(formerlypublic), copied from thegridiconsrepo.svg4everybodypolyfill to support SVG external content in old browsersTesting instructions
calypso.livebranch of this PR in one browser tab.