[Doc] Convert Toolbars to use the new standard. #2415
Conversation
| @@ -60,7 +72,7 @@ const ToolbarTitle = React.createClass({ | |||
| }, style); | |||
|
|
|||
| return ( | |||
| <span style={styles} {...other} >{text}</span> | |||
| <span className={this.props.className} style={styles} {...other} >{text}</span> | |||
There was a problem hiding this comment.
The class is already inside the ...other could you extract it using the spread?
There was a problem hiding this comment.
I also think that it's more wisely to put the {...other} at the beginning.
|
@oliviertassinari Please check again. Is this better? |
| import ToolbarSeparator from 'material-ui/lib/toolbar/toolbar-separator'; | ||
| import ToolbarTitle from 'material-ui/lib/toolbar/toolbar-title'; | ||
|
|
||
| let menuItems = [ |
|
You must have seen that Remarque regarding the documentation:
|
|
@oliviertassinari Thanks for the review. I will fix them asap. And regarding
You already did this 😁
I will try to think of something. 👍 |
|
@oliviertassinari Please review again. I think coding styles must be kept consistent, I did some aggressive changes 😈 |
|
That's pretty agressive indeed. |
| @@ -46,21 +67,33 @@ const Toolbar = React.createClass({ | |||
| return this.state.muiTheme.toolbar; | |||
| }, | |||
|
|
|||
| getStyles() { | |||
There was a problem hiding this comment.
You have a point, alright.
|
@oliviertassinari Please check again |
| @@ -46,21 +67,36 @@ const Toolbar = React.createClass({ | |||
| return this.state.muiTheme.toolbar; | |||
| }, | |||
|
|
|||
| getSpacing() { | |||
There was a problem hiding this comment.
I suppose that the getSpacing and the getTheme are only called in the getStyles.
In this case, shouldn't we remove those two methods and add two variables in the getStyles?
There was a problem hiding this comment.
My thoughts exactly!! But this is somehow a bit global. I wanted to keep consistent with rest and propose this later on. If you agree, and follow the same path, we'll enforce some code styles as we migrate the docs and their corresponding components.
|
@subjectix Thanks |
[Doc] Convert Toolbars to use the new standard.
|
Thanks 🎉 🎉 |
@oliviertassinari I had to make some modifications to your
PropTypeDescriptionto support multiple components per page. Also I did some improvements to fix some bugs, added className toToolbarTitleto ensure consistency and migrated frommenutomenus. Please review.One last thing to note. seems like the new
menuscannot be corretly positioned in aToolbarthe IconButton is not in the right place. But that doesn't have anything to do with this PR. merge if it's alright with you.