Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Material icons package and svgr #32236

Closed
wants to merge 9 commits into from

Conversation

griffbrad
Copy link
Contributor

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 these icons, I'm using svgr, which is also under consideration in core/Gutenberg (WordPress/gutenberg#14628). This loads the SVG with webpack and transforms it into a React component.

The only place where we load SVGs currently is client/lib/flags, which required a minor update to continue working as expected.

This isolates the dependency changes from the remainder of the nav drawer work, which hopefully will aid review and ease merging.

Testing instructions

  • With these changes, you should be able to load an SVG and get a React component. import { ReactComponent as SvgTest } from 'material-design-icons/action/outline-offline_bolt-24px.svg';.
  • The flag tests should still pass.

In order to support the flagUrl() library function, we now mock svgr's
return value, including the "default" export with the URL to the SVG
file.  This allows us to continue importing the ReactComponent for an
SVG while also supporting use cases like flagUrl().
@griffbrad griffbrad added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 11, 2019
@griffbrad griffbrad requested a review from blowery April 11, 2019 21:28
@griffbrad griffbrad requested a review from a team as a code owner April 11, 2019 21:28
@matticbot
Copy link
Contributor

@sgomes
Copy link
Contributor

sgomes commented Apr 16, 2019

Great work, @griffbrad ! SVGR is a much simpler pipeline than what we're currently doing in e.g. gridicons.

I'm wondering what the expected use of these Material icons is going to be in the future, however. If we expect to see widespread usage of many icons across the application, I'd argue against this SVG-in-JS approach. JS is byte-for-byte much more expensive than images, since it requires additional parsing and compilation, even if it's not executed. In addition, wrapping these icons in JS invalidates any performance oprimisations the browser would otherwise be able to perform, such as deprioritising download vs other resources, and would hurt cacheability if bundled in with the rest of the JS.

If you're expecting significant usage, I'd argue for either an SVG spritemap approach (provides a good balance between cacheability and number of HTTP requests), or a plain one-SVG-file-per-icon approach (optimum cacheability, but potentially large number of requests, which could be problematic for HTTP 1.x connections). Both of these keep images as images, allowing the browser to do its thing well. The icons are still styleable if you make use of SVG <use> for loading the external resource (polyfillable in IE11 with svg4everybody). The React component then becomes an extremely simple bit of generic embedded SVG that references the external icon.

This is what I'm currently experimenting with in #32172 for gridicons, and appears to be working well.

@griffbrad
Copy link
Contributor Author

I like the gridicons approach in #32172 with svg4everybody. Will get this PR switched over to be compatible with that approach and ping you to review. Thanks for the pointer!

@griffbrad
Copy link
Contributor Author

This PR is superseded by PR #32364, which uses the svg4everybody polyfill rather than svgr.

@griffbrad griffbrad closed this Apr 17, 2019
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 17, 2019
@lancewillett lancewillett deleted the add/material-design-icons-package branch March 9, 2020 22:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants