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] Increase clarity around the usage of font icons #13628

Merged
merged 2 commits into from
Nov 17, 2018
Merged

[docs] Increase clarity around the usage of font icons #13628

merged 2 commits into from
Nov 17, 2018

Conversation

JosephMart
Copy link
Contributor

@JosephMart JosephMart commented Nov 17, 2018

With this current sample code, I was not able to get SendIcon to work.

I was able to import SendIcon from'@material-ui/core/Send' and use it to display the SendIcon within the button.

With this current sample code, I was not able to get `SendIcon` to work.

I was able to import `SendIcon` from`'@material-ui/core/Send'` and use it to display the `SendIcon` within the button.
@oliviertassinari oliviertassinari self-assigned this Nov 17, 2018
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation component: button This is the name of the generic UI component, not the React module! labels Nov 17, 2018
@oliviertassinari oliviertassinari changed the title Fixed bug where sample code for SendIcon [docs] Increase clarity around the usage of font icons Nov 17, 2018
@oliviertassinari
Copy link
Member

@JosephMart Thank you for raising the concern, I have added a comment to guide people.

@oliviertassinari oliviertassinari merged commit 66e32ed into mui:master Nov 17, 2018
@mbrookes
Copy link
Member

mbrookes commented Nov 17, 2018

@oliviertassinari This is why we eliminated font icon examples from the docs, (other than in the Icon section). They don't work out-of-the box, and cause this kind of confusion.

Any idea why this one is a font icon?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 17, 2018

@mbrookes It demonstrates both solutions can be switched, same output.

@mbrookes
Copy link
Member

We already have the Icons section for that, whereas this example has zero discoverability. It seems a bit random to stick a font icon in here.

Your call.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 18, 2018

As long as we have a visual regression test that compares font icons with svg icons, side by side in a button, I'm happy.

@mbrookes
Copy link
Member

visual regression test

Aha, right.

If this comes up again, I'll create s dedicated regression test and remove the font icon from the Button examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: button This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants