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.

Sign up for GitHub

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

Jump to bottom

[Docs] Icon's docs update #2695

Merged
merged 1 commit into from
Jan 5, 2016
Merged

[Docs] Icon's docs update #2695

merged 1 commit into from
Jan 5, 2016

Conversation

Zadielerick
Copy link
Contributor

Updated Documentation for Icons, which includes Font Icons and SVG Icons

@oliviertassinari
Copy link
Member

@Zadielerick You have a conflict with this PR.

oliviertassinari
oliviertassinari reviewed Dec 29, 2015
View reviewed changes
docs/src/app/components/pages/components/Icons/ExampleHover.jsx
hoverColor={Colors.greenA200} />
<FontIcon className="muidocs-icon-action-home" style={iconStyles}
color={Colors.red500} hoverColor={Colors.greenA200} />
<FontIcon className="muidocs-icon-action-home" style={iconStyles}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need 4 FontIcon? I think that 2 is enough.

@oliviertassinari
Copy link
Member

What do you think about breaking the Icons page in 2 pages?

  • FontIcon
  • SvgIcon

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation PR: Needs Review labels Dec 29, 2015
@Zadielerick
Copy link
Contributor Author

@oliviertassinari Yeah, I thought about that since they were both crammed into one.
sounds good to me

@Zadielerick
Copy link
Contributor Author

@oliviertassinari Now that I think about it, this also happens in the buttons with the 3 buttons(floating action, rasied, flat). Is there a way we are going to handle this? Or should I just split this up into two separate pages for now?

@oliviertassinari
Copy link
Member

this also happens in the buttons with the 3 buttons(floating action, rasied, flat)

Yeah, I think that we should also split up into 3 pages for the buttons.

Is there a way we are going to handle this?

Following #2690, we can create a third level:
Components/buttons/flat-button,...

@Zadielerick
Copy link
Contributor Author

@oliviertassinari Nice, got it, thanks!

oliviertassinari
oliviertassinari reviewed Dec 29, 2015
View reviewed changes
docs/src/app/components/pages/components/Icons/ExampleSVG.jsx

const IconsExampleSVGIcons = () => (
<div>
<ActionHome style={iconStyles} color={Colors.yellow500} hoverColor={Colors.greenA200} />
Copy link
Member

Choose a reason for hiding this comment

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

x2 😁

@oliviertassinari oliviertassinari mentioned this pull request Dec 30, 2015
Closed
29 tasks
oliviertassinari
oliviertassinari reviewed Dec 30, 2015
View reviewed changes
docs/src/app/app-routes.jsx
@@ -103,6 +104,7 @@ const AppRoutes = (
<Route path="refresh-indicator" component={RefreshIndicator} />
<Route path="select-field" component={SelectField} />
<Route path="sliders" component={Sliders} />
<Route path="svg-icon" component={SVGIconPage} />
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the capital SVGIcon, the component displayName is SvgIcon.
Shouldn't we follow this?

@oliviertassinari
Copy link
Member

@Zadielerick That's definitely better with the Icon section in the LeftNav 👍.

@Zadielerick
Copy link
Contributor Author

@oliviertassinari Yeah, I like the new left nav layout! Great job!

oliviertassinari
oliviertassinari reviewed Dec 30, 2015
View reviewed changes
docs/src/app/components/pages/components/FontIcon/README.md
<link href="https://fonts.googleapis.com/icon?family=Material+Icons" rel="stylesheet">
```

To see available Material Icons, go to [material icons library](https://design.google.com/icons/). The names are in snake_case format, for example: find_in_page
Copy link
Member

Choose a reason for hiding this comment

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

snake_case or find_in_page what's this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When calling google's material icons, their names are stored using that convention instead of camel case. For example, instead of ActionGrade, we would use action-grade when we wanted to call that svg from google's icons. I guess it should be phrased better

Copy link
Member

Choose a reason for hiding this comment

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

Ok thanks. Can we use action_home example in this case?

@oliviertassinari
Copy link
Member

@Zadielerick I think that we can reorganize the examples.
What do you think about having 2 examples for FontIcon and 2 examples for SvgIcon?
They would have the same output but different implementation.

The first example would have:

  • a simple house,
  • a house with a color property
  • a house with a color and an hoverColor property

The second example would have:

  • a simple house
  • a simple flight taking of
  • a simple cloud download
  • a simple video game asset

@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: Needs Review labels Dec 31, 2015
oliviertassinari
oliviertassinari reviewed Jan 1, 2016
View reviewed changes
docs/src/app/components/pages/components/SVGIcon/README.md
@@ -0,0 +1,5 @@
## SVG Icons
Using material-ui's SVG Icon, we can create a custom [svg component](https://www.google.com/design/spec/style/icons.html#icons-system-icons). In our examples, we are creating the ActionHome SvgIcon for this docs site, and using it in some separate component.
Custom SvgIcon components can be included as children for other Material UI components that use icons such as [IconButtons](http://www.material-ui.com/#/components/icon-buttons).
Copy link
Member

Choose a reason for hiding this comment

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

The link changed from /icon-buttons to /icon-button.

@Zadielerick Zadielerick force-pushed the iconsDocsUpdate branch 2 times, most recently from 7cfe4fa to 024c6d4 Compare January 4, 2016 14:33
@oliviertassinari oliviertassinari mentioned this pull request Jan 4, 2016
Merged
@Zadielerick Zadielerick force-pushed the iconsDocsUpdate branch 2 times, most recently from 282a1f3 to bfc33b3 Compare January 4, 2016 19:40
@oliviertassinari
Copy link
Member

@Zadielerick That's some hard work, see my last component, I think that we can merge then 👍.

oliviertassinari added a commit that referenced this pull request Jan 5, 2016
@oliviertassinari
Merge pull request #2695 from Zadielerick/iconsDocsUpdate
3d0c367 
[Docs] Icon's docs update
@oliviertassinari oliviertassinari merged commit 3d0c367 into mui:master Jan 5, 2016
@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jan 5, 2016
@Zadielerick Zadielerick deleted the iconsDocsUpdate branch January 6, 2016 00:03
@oliviertassinari oliviertassinari mentioned this pull request Jan 31, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
docs Improvements or additions to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Zadielerick @oliviertassinari