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

Add/material design icons and svg4everybody #32364

Merged
merged 18 commits into from
Apr 17, 2019

Conversation

griffbrad
Copy link
Contributor

@griffbrad griffbrad commented Apr 17, 2019

Changes proposed in this Pull Request

Add a package for storing the Material icons we're using in the new nav drawer work (see PR #31742). The official material-design-icon package (https://github.com/google/material-design-icons) has not been updated since Jan 2018. It is missing several icons we intend to use.

To load SVGs, I use the technique @sgomes introduced in PR #32172. This uses svg4everybody to polyfill the use of external references in SVG <use> elements, enabling the browser to load and optimize SVG rather than converting SVGs to React components.

Testing instructions

With this PR, you can use the new MaterialIcon component to render an icon. Example:

import MaterialIcon from 'components/material-icon';  

const Icon = <MaterialIcon size={ 24 } icon="offline_bolt" />;

@griffbrad griffbrad requested review from blowery and sgomes April 17, 2019 18:18
@griffbrad griffbrad requested a review from a team as a code owner April 17, 2019 18:18
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Apr 17, 2019

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

Webpack Runtime
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.

name      parsed_size    gzip_size
manifest         +0 B        +12 B  (+0.0%)

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

name   parsed_size           gzip_size
build        +84 B  (+0.0%)       +4 B  (+0.0%)

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

name                          parsed_size           gzip_size
async-load-design-playground       +772 B  (+0.0%)     +300 B  (+0.1%)
async-load-design-blocks           +772 B  (+0.0%)     +412 B  (+0.1%)
async-load-design                  +772 B  (+0.0%)     +480 B  (+0.1%)

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.

package.json Outdated Show resolved Hide resolved
webpack.config.js Outdated Show resolved Hide resolved
@blowery
Copy link
Contributor

blowery commented Apr 17, 2019

@griffbrad I added the MaterialIcon to the playground scope just to be able to test it out there. Looks good! We might want to add an example to devdocs too, but that can probably wait for another PR.

Copy link
Contributor

@blowery blowery left a comment

Choose a reason for hiding this comment

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

LGTM

@griffbrad griffbrad merged commit 079f3e9 into master Apr 17, 2019
populate this package with all the available icons. Alternatively, we could seek
out an alternative distribution of the icons.

## Using a Material icon in Calypso
Copy link
Contributor

Choose a reason for hiding this comment

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

@griffbrad Looks like the readme needs to be updated in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up. Will post that later today.

import classnames from 'classnames';
import React from 'react';
import PropTypes from 'prop-types';
import spritePath from '@automattic/material-design-icons/svg-sprite/material-icons.svg';
Copy link
Contributor

Choose a reason for hiding this comment

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

@griffbrad This line is interesting. How does this import get converted into a path string, and how would I be able to replicate it in gridicons?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@griffbrad griffbrad Apr 18, 2019

Choose a reason for hiding this comment

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

This is done with webpack's file-loader. In the context of Calypso, it's added to webpack.config.js here:
https://github.com/Automattic/wp-calypso/blob/master/webpack.config.js#L234

That comes out of the Calypso build package:
https://github.com/Automattic/wp-calypso/blob/master/packages/calypso-build/webpack/file-loader.js

Copy link
Contributor

Choose a reason for hiding this comment

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

Very interesting, thank you both! Should be easy to replicate for gridicons, then :)

@sgomes
Copy link
Contributor

sgomes commented Apr 18, 2019

Excellent work, @griffbrad, some great improvements on my experimental PR! I'll be sure to copy what you did here when I start work on gridicons 🙂

@paaljoachim
Copy link

I am thinking that you might have some good input on the Add icons block being proposed for Gutenberg Core. WordPress/gutenberg#16484

@sgomes
Copy link
Contributor

sgomes commented Aug 14, 2019

Thanks, @paaljoachim, I'll take a look!

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.

5 participants