Reimplement gridicons using SVG external content. #32763

sgomes · 2019-05-02T15:46:46Z

Reimplement gridicons using an SVG external content approach, instead of SVG transpiled into React components. This helps keep images as images (which is good for performance), while maintaining styleability. Old browsers are supported through the already-included svg4everyone polyfill.

This PR supersedes and replaces #32172.

See #32364 for a similar change, which added Material Icons with the same approach.

Changes proposed in this Pull Request

Remove AsyncGridicons component.
Add Gridicons component, using offset data and SVG sprite from gridicons NPM package.
Bump gridicons NPM package version to one that includes the new files.
Modify gridicon CSS rules to be compatible with this approach.

Testing instructions

Open the calypso.live branch of this PR in one browser tab.
Open production in another browser tab.
Alternate between both tabs, comparing as many different Calypso pages as you see fit. They should all be identical, pixel for pixel.

matticbot · 2019-05-02T15:46:47Z

Test live: https://calypso.live/?branch=update/gridicons-from-external-svg

matticbot · 2019-05-02T16:17:28Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Webpack Runtime (~360 bytes removed 📉)

name      parsed_size           gzip_size
manifest       -360 B  (-0.2%)     -105 B  (-0.4%)

Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded.

App Entrypoints (~44696 bytes removed 📉)

name            parsed_size           gzip_size
build              -22397 B  (-1.3%)    -2401 B  (-0.5%)
domainsLanding     -22299 B  (-3.6%)    -2400 B  (-1.4%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Legacy SCSS Stylesheet (~157 bytes added 📈)

name       parsed_size           gzip_size
style.css       +157 B  (+0.0%)      +20 B  (+0.0%)

The monolithic CSS stylesheet that is downloaded on every app load.
👎 This PR increases the size of the stylesheet, which is a bad news. Please consider migrating the CSS styles you modified to webpack imports.

Sections (~20237 bytes removed 📉)

name                   parsed_size           gzip_size
post-editor               -11215 B  (-0.5%)    -2314 B  (-0.5%)
checkout                   -2453 B  (-0.3%)    -1261 B  (-0.7%)
woocommerce                -1423 B  (-0.1%)     -595 B  (-0.1%)
plans                      -1240 B  (-0.3%)     -541 B  (-0.5%)
stats                       -182 B  (-0.0%)      -22 B  (-0.0%)
domains                     -168 B  (-0.0%)      -50 B  (-0.0%)
purchases                   -161 B  (-0.0%)      -32 B  (-0.0%)
plugins                     -161 B  (-0.0%)      -31 B  (-0.0%)
settings-writing            -140 B  (-0.0%)      -24 B  (-0.0%)
posts-pages                 -140 B  (-0.0%)      -29 B  (-0.0%)
settings                    -133 B  (-0.0%)      -27 B  (-0.0%)
reader                      -133 B  (-0.0%)       -7 B  (-0.0%)
jetpack-connect             -133 B  (-0.0%)      -26 B  (-0.0%)
gutenberg-editor            -133 B  (-0.0%)      -29 B  (-0.0%)
posts-custom                -126 B  (-0.0%)      -23 B  (-0.0%)
help                        -126 B  (-0.0%)      -21 B  (-0.0%)
security                    -119 B  (-0.0%)      -37 B  (-0.0%)
activity                    -119 B  (-0.0%)      -20 B  (-0.0%)
media                       -112 B  (-0.0%)      -26 B  (-0.0%)
settings-security           -105 B  (-0.0%)      -24 B  (-0.0%)
email                       -105 B  (-0.0%)      -27 B  (-0.0%)
themes                       -98 B  (-0.0%)      -25 B  (-0.0%)
people                       -98 B  (-0.0%)      -13 B  (-0.0%)
notification-settings        -98 B  (-0.0%)      -23 B  (-0.0%)
marketing                    -98 B  (-0.0%)      -21 B  (-0.0%)
comments                     -98 B  (-0.0%)      -20 B  (-0.0%)
account                      -98 B  (-0.0%)      -29 B  (-0.0%)
theme                        -91 B  (-0.0%)      -17 B  (-0.0%)
earn                         -84 B  (-0.0%)      -13 B  (-0.0%)
devdocs                      +84 B  (+0.1%)       -2 B  (-0.0%)
google-my-business           -77 B  (-0.0%)      -18 B  (-0.0%)
account-close                -77 B  (-0.0%)      -21 B  (-0.0%)
wp-super-cache               -70 B  (-0.0%)      -13 B  (-0.0%)
site-blocks                  -63 B  (-0.0%)      -22 B  (-0.0%)
settings-performance         -63 B  (-0.0%)      -15 B  (-0.0%)
settings-discussion          -63 B  (-0.0%)      -15 B  (-0.0%)
privacy                      -63 B  (-0.0%)      -22 B  (-0.0%)
me                           -63 B  (-0.0%)      -22 B  (-0.0%)
happychat                    -63 B  (-0.0%)      -22 B  (-0.0%)
zoninator                    -56 B  (-0.0%)       -7 B  (-0.0%)
feature-upsell               -42 B  (-0.0%)       -9 B  (-0.0%)
concierge                    -42 B  (-0.0%)      -13 B  (-0.0%)
preview                      -28 B  (-0.0%)       -9 B  (-0.0%)
login                        -28 B  (-0.0%)       -9 B  (-0.0%)
customize                    -28 B  (-0.0%)      -12 B  (-0.0%)
sites                        -21 B  (-0.0%)       -3 B  (-0.0%)
sensei                       -21 B  (-0.0%)       -3 B  (-0.0%)
jetpack-onboarding           -21 B  (-0.0%)       -5 B  (-0.0%)
hello-dolly                  -21 B  (-0.0%)       -3 B  (-0.0%)
accept-invite                -21 B  (-0.0%)       -7 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~118369 bytes removed 📉)

name                                                         parsed_size           gzip_size
async-load-design                                              -115505 B  (-6.2%)   -20554 B  (-4.9%)
async-load-design-blocks                                         -1576 B  (-0.1%)     -565 B  (-0.1%)
async-load-design-playground                                      -252 B  (-0.0%)      -30 B  (-0.0%)
async-load-components-web-preview-component                       -105 B  (-0.0%)      -15 B  (-0.0%)
async-load-signup-steps-domains                                    -70 B  (-0.0%)      -12 B  (-0.0%)
async-load-signup-steps-clone-point                                -70 B  (-0.0%)       -7 B  (-0.0%)
async-load-post-editor-media-modal                                 -63 B  (-0.0%)      -20 B  (-0.0%)
async-load-blocks-inline-help-popover                              -56 B  (-0.0%)       -4 B  (-0.0%)
async-load-signup-steps-plans-without-free                         -49 B  (-0.0%)      -13 B  (-0.0%)
async-load-signup-steps-plans                                      -49 B  (-0.0%)      -13 B  (-0.0%)
async-load-signup-steps-rewind-form-creds                          -35 B  (-0.1%)       -7 B  (-0.1%)
async-load-reader-following-manage                                 -35 B  (-0.0%)       -5 B  (-0.0%)
async-load-extensions-woocommerce-app-store-stats-referrers        -35 B  (-0.1%)       -4 B  (-0.0%)
async-load-extensions-woocommerce-app-store-stats                  -35 B  (-0.1%)       -8 B  (-0.1%)
async-load-signup-steps-user                                       -28 B  (-0.0%)      -12 B  (-0.1%)
async-load-signup-steps-theme-selection                            -28 B  (-0.1%)       -5 B  (-0.1%)
async-load-signup-steps-plans-atomic-store                         -28 B  (-0.0%)       -9 B  (-0.0%)
async-load-reader-site-stream                                      -28 B  (-0.1%)       -4 B  (-0.0%)
async-load-reader-feed-stream                                      -28 B  (-0.1%)       -4 B  (-0.1%)
async-load-quick-language-switcher                                 -28 B  (-0.1%)       -3 B  (-0.0%)
async-load-signup-steps-site-topic                                 -21 B  (-0.1%)       -7 B  (-0.1%)
async-load-signup-steps-site-title                                 -21 B  (-0.1%)       -4 B  (-0.1%)
async-load-signup-steps-clone-credentials                          -21 B  (-0.1%)       -8 B  (-0.1%)
async-load-signup-steps-about                                      -21 B  (-0.1%)       -3 B  (-0.0%)
async-load-my-sites-site-settings-section-import                   -21 B  (-0.0%)       -4 B  (-0.0%)
async-load-extensions-woocommerce-app-store-stats-listview         -21 B  (-0.1%)       -3 B  (-0.1%)
async-load-signup-steps-survey                                     -14 B  (-0.1%)       -2 B  (-0.1%)
async-load-signup-steps-site-style                                 -14 B  (-0.1%)       -3 B  (-0.1%)
async-load-signup-steps-site                                       -14 B  (-0.1%)      -11 B  (-0.3%)
async-load-signup-steps-clone-destination                          -14 B  (-0.1%)       -5 B  (-0.1%)
async-load-reader-tag-stream-main                                  -14 B  (-0.1%)       -4 B  (-0.1%)
async-load-reader-sidebar                                          -14 B  (-0.0%)       -9 B  (-0.1%)
async-load-reader-search-stream                                    -14 B  (-0.0%)       -5 B  (-0.0%)
async-load-my-sites-site-settings-section-export                   -14 B  (-0.1%)       -3 B  (-0.0%)
async-load-my-sites-current-site-domain-warnings                   -14 B  (-0.0%)       -4 B  (-0.0%)
async-load-blocks-reader-full-post                                 -14 B  (-0.0%)       -4 B  (-0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Gridicons (~228362 bytes removed 📉)

name       parsed_size             gzip_size
gridicons    -228362 B  (deleted)   -22417 B  (deleted)

Set of SVG icons that is loaded asynchronously to not delay the initial load. Unless you are modifying Gridicons, you should not see any change here.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

sgomes · 2019-05-03T13:35:12Z

~~Blocking this PR on #32750, as there's a bit of duplicate work between the two.~~

#32750 has now been merged, so this PR is ready for review.

jsnajdr

There's a little glitch with needs-offset and IE11.

I also have one question: when using the use xlink:href reference, can we target elements inside the SVG with CSS? I think we might use it at a few places, I'm not sure.

client/components/tinymce/plugins/media/advanced/index.jsx

jsnajdr · 2019-05-06T11:40:05Z

assets/stylesheets/_vendor.scss

@@ -5,15 +5,15 @@
 .gridicon {
 	fill: currentColor;

-	&.needs-offset g {
+	&.needs-offset use {


If the svg4everybody polyfill is active, there won't be any use elements to style. The polyfill will remove them and replace with the SVG markup it downloads with XHR.

We need a style that targets both g and use elements.

Excellent catch, thanks @jsnajdr !

I have pushed a fix for this using :first-child. Waiting for calypso.live before testing on IE11.

The fix appears to be working, from what I can tell.

sgomes · 2019-05-06T12:23:33Z

I also have one question: when using the use xlink:href reference, can we target elements inside the SVG with CSS? I think we might use it at a few places, I'm not sure.

We can't, no; it's effectively a shadow DOM.

I couldn't find any instances of this targeting other than for alignment, which I rewrote in CSS.

jsnajdr · 2019-05-06T12:41:43Z

I couldn't find any instances of this targeting other than for alignment, which I rewrote in CSS.

Yes, I also found only instances where we style the top-level g element, usually changing color or offset. These selectors seems to be changed to target use everywhere.

This ensures that alignment works both in polyfilled and non-polyfilled setups.

jsnajdr · 2019-05-06T14:38:15Z

assets/stylesheets/_vendor.scss

@@ -5,15 +5,15 @@
 .gridicon {
 	fill: currentColor;

-	&.needs-offset g {
+	&.needs-offset :first-child {


When the whole SVG is inline rather than referenced, doesn't this also offset all nested elements that happen to be the first child of their parent?

&.needs-offset > g:first-child, &.needs-offset > use:first-child { transform: ... }

should be more reliable. What do you think?

Good catch, once again! But if the concern is to restrict to immediate descendants, then the following should suffice:

&.needs-offset > :first-child { transform: ... }

Would you prefer for me to still explicitly mention g and use?

Would you prefer for me to still explicitly mention g and use?

Not really needed, just more safe and explicit IMO 🙂

No worries, I'l switch to that, then :)

jsnajdr

👍 🎉

When Gridicons were redone in #32763, they switched from being Class components to Functional components. Functional components cannot be the target of a React ref unless the ref is forwarded using React.createRef. We use Gridicons by ref a number of places in Calypso as popover contexts. Add ref forwarding to fix various places.

per #32763

Per prop change in #32763 - Change CartSummaryBar GridIcon size to numeric

sgomes added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Blocked / Hold labels May 2, 2019

sgomes mentioned this pull request May 2, 2019

Experiment: load gridicons from external SVG file. #32172

Closed

sgomes force-pushed the update/gridicons-from-external-svg branch from c5e2203 to 1ccb040 Compare May 3, 2019 11:45

sgomes added 5 commits May 6, 2019 09:38

Reimplement gridicons using SVG external content.

3476e36

Replace external Gridicon example w/ internal one.

5d892f3

Update icons.md with new gridicons info.

934410e

Replace usage of 'gridicons/dist/...'

ec9b022

Enable SVG file-loader transform in gridicons.

21625f8

sgomes force-pushed the update/gridicons-from-external-svg branch from b677ab1 to 21625f8 Compare May 6, 2019 08:41

sgomes requested a review from a team May 6, 2019 08:59

sgomes added [Type] Performance and removed [Status] Blocked / Hold labels May 6, 2019

jsnajdr reviewed May 6, 2019

View reviewed changes

sgomes added 2 commits May 6, 2019 14:05

Change icon adjustment CSS to use :first-child.

508d4eb

This ensures that alignment works both in polyfilled and non-polyfilled setups.

Remove role=presentation from button.

01a0103

jsnajdr reviewed May 6, 2019

View reviewed changes

Fix scoping bug in updated CSS rules.

804f39b

jsnajdr approved these changes May 6, 2019

View reviewed changes

Make CSS rules more explicit

e2e1b63

sgomes merged commit 5ea609d into master May 6, 2019

sgomes deleted the update/gridicons-from-external-svg branch May 6, 2019 15:15

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 6, 2019

blowery mentioned this pull request May 6, 2019

Gridicons: Forward refs on gridicons to the wrapped svg #32840

Merged

lsl added a commit that referenced this pull request May 8, 2019

Change CartSummaryBar GridIcon size to numeric

c4808d4

per #32763

lsl mentioned this pull request May 8, 2019

Change CartSummaryBar GridIcon size to numeric #32883

Merged

lsl added a commit that referenced this pull request May 10, 2019

Change CartSummaryBar GridIcon size to numeric (#32883)

2ead433

Per prop change in #32763 - Change CartSummaryBar GridIcon size to numeric

sirreal mentioned this pull request Sep 20, 2019

SSR: Fix file-loader assets #36204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement gridicons using SVG external content. #32763

Reimplement gridicons using SVG external content. #32763

sgomes commented May 2, 2019 •

edited

Loading

matticbot commented May 2, 2019

matticbot commented May 2, 2019 •

edited

Loading

sgomes commented May 3, 2019 •

edited

Loading

jsnajdr left a comment

jsnajdr May 6, 2019

sgomes May 6, 2019

sgomes May 6, 2019

sgomes commented May 6, 2019

jsnajdr commented May 6, 2019

jsnajdr May 6, 2019

sgomes May 6, 2019 •

edited

Loading

jsnajdr May 6, 2019

sgomes May 6, 2019

jsnajdr left a comment

Reimplement gridicons using SVG external content. #32763

Reimplement gridicons using SVG external content. #32763

Conversation

sgomes commented May 2, 2019 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented May 2, 2019

matticbot commented May 2, 2019 • edited Loading

sgomes commented May 3, 2019 • edited Loading

jsnajdr left a comment

Choose a reason for hiding this comment

jsnajdr May 6, 2019

Choose a reason for hiding this comment

sgomes May 6, 2019

Choose a reason for hiding this comment

sgomes May 6, 2019

Choose a reason for hiding this comment

sgomes commented May 6, 2019

jsnajdr commented May 6, 2019

jsnajdr May 6, 2019

Choose a reason for hiding this comment

sgomes May 6, 2019 • edited Loading

Choose a reason for hiding this comment

jsnajdr May 6, 2019

Choose a reason for hiding this comment

sgomes May 6, 2019

Choose a reason for hiding this comment

jsnajdr left a comment

Choose a reason for hiding this comment

sgomes commented May 2, 2019 •

edited

Loading

matticbot commented May 2, 2019 •

edited

Loading

sgomes commented May 3, 2019 •

edited

Loading

sgomes May 6, 2019 •

edited

Loading