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

[docs] Add explanation for using font icon in a local project #6198

Merged
merged 4 commits into from
Feb 22, 2017
Merged

[docs] Add explanation for using font icon in a local project #6198

merged 4 commits into from
Feb 22, 2017

Conversation

Shahrukh-Zindani
Copy link

@Shahrukh-Zindani Shahrukh-Zindani commented Feb 20, 2017

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@oliviertassinari oliviertassinari changed the title [Docs] Add a link FontIcon component to explain the process of using … [docs] Explain the process of using font icon in a local project Feb 20, 2017
@oliviertassinari oliviertassinari changed the title [docs] Explain the process of using font icon in a local project [docs] Explains the process of using font icon in a local project Feb 20, 2017
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation PR: Review Accepted and removed PR: Needs Review labels Feb 20, 2017
@mbrookes mbrookes changed the title [docs] Explains the process of using font icon in a local project [docs] Add explanation for using font icon in a local project Feb 21, 2017
@mbrookes mbrookes added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Review Accepted labels Feb 21, 2017
@mbrookes
Copy link
Member

The linked article:

  • Declares itself out-of-date
  • Makes the process seem unnecessarily complicated.

@Shahrukh-Zindani
Copy link
Author

I had no idea of how to use font icon until I read this article. The intention was to make it easy for users to be able to use font icons quickly in their projects.

@Shahrukh-Zindani
Copy link
Author

Changed the article link to a different article which looks good to me.

@mbrookes
Copy link
Member

Perhaps something more authoritative than a random blog post? http://google.github.io/material-design-icons/#icon-font-for-the-web

@mbrookes mbrookes added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 21, 2017
@@ -4,7 +4,7 @@ This component will render any [icon](https://www.google.com/design/spec/style/i
defined in any stylesheet included in your project. You can use sites like [IcoMoon](https://icomoon.io/)
to generate custom font files, or use prebuilt fonts such as [Material icons](https://design.google.com/icons/) or
[Font Awesome](http://fontawesome.io/) either included in your project, or served from a public
[Content Delivery Network](https://en.wikipedia.org/wiki/Content_delivery_network).
[Content Delivery Network](https://en.wikipedia.org/wiki/Content_delivery_network). Refer to this [article](http://google.github.io/material-design-icons/#icon-font-for-the-web) to learn how to use font icons from any of the above websites in your project.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this affecting the CDN line?

Copy link
Author

Choose a reason for hiding this comment

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

Might have been copied and pasted. Will be careful about it. It does not change the look though as that line was added back.

Copy link
Member

Choose a reason for hiding this comment

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

Please use a line break. Having a line > 100 character is bad for readability & collaboration.

@oliviertassinari oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Review Accepted labels Feb 21, 2017
@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 22, 2017
@oliviertassinari oliviertassinari merged commit 3ca4716 into mui:master Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants