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

fix(material-icon): removed blocking line and updated documentation #155

Merged
merged 7 commits into from
Jul 21, 2018

Conversation

moog16
Copy link

@moog16 moog16 commented Jul 19, 2018

fixes #98

@codecov-io
Copy link

codecov-io commented Jul 19, 2018

Codecov Report

Merging #155 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #155   +/-   ##
=======================================
  Coverage   95.78%   95.78%           
=======================================
  Files          21       21           
  Lines         712      712           
  Branches       64       64           
=======================================
  Hits          682      682           
  Misses         30       30

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1569f97...3ef6c0e. Read the comment docs.

#### Importing Icons

The React material icon package does not come packaged with [Google Font's Material Icons](https://google.github.io/material-design-icons/). If it was included in the Sass package this would block rendering of the page until the icons download.
We recommend following the two methods of adding the Material Icons to your app documented [here](https://google.github.io/material-design-icons/#getting-icons). We have personally taken the `<link>` tag approach as it doesn't block page render. Code is pasted here for your convenience.
Copy link
Contributor

Choose a reason for hiding this comment

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

"following the two methods" ... this is unclear. Do you mean the developer has to do two things? Or the developer should pick one way or another. And which one is the <link> approach?

Copy link
Author

Choose a reason for hiding this comment

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

There are 2 ways that are documented within on material icons. How about...

We recommend following either of the 2 ways of adding Material Icons to your app, which are both documented ....

The 1st documented solution is the route our team has taken, since its not bundled with our CSS. This allows for quicker rendering, but a possibility of a Flash Of Unstyled Content (**FOUC**).

...require('./fab/webpack.config.js'),
...require('./floating-label/webpack.config.js'),
...require('./line-ripple/webpack.config.js'),
// ...require('./button/webpack.config.js'),
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be in this PR?

The React material icon package does not come packaged with [Google Font's Material Icons](https://google.github.io/material-design-icons/). If it was included in the Sass package this would block rendering of the page until the icons download.


We recommend following either of the 2 ways of adding Material Icons to your app, which are both documented [here](https://google.github.io/material-design-icons/#icon-font-for-the-web). The 1st documented solution is the route our team has taken, since its not bundled with our CSS. This allows for quicker rendering, but a possibility of a Flash Of Unstyled Content (**FOUC**). Code is pasted here for your convenience.
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two different ways to add Material Icons to your website, both of which are documented in Material Icons Documentation. We recommend "Method 1: via Google Web Fonts", because it renders faster, but there can be Flash Of Unstyled Content (FOUC). The code to load Material Icons via Google Web Fonts is pasted here for your convenience.

@moog16 moog16 merged commit 8f3cef5 into master Jul 21, 2018
@moog16 moog16 deleted the fix/material-icon/dont-block-page-render branch July 21, 2018 00:51
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.

3 participants