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 documentation for containerElement in RaisedButon #6199

Merged
merged 4 commits into from
Feb 22, 2017
Merged

[docs] Add documentation for containerElement in RaisedButon #6199

merged 4 commits into from
Feb 22, 2017

Conversation

Shahrukh-Zindani
Copy link

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

Documentation for containerElement in RaisedButton.
Close issue #5647

@@ -134,6 +134,13 @@ class RaisedButton extends Component {
*/
className: PropTypes.string,
/**
* This is the Link tag that can be used inside RaisedButton.
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't sound quite right...

Copy link
Author

Choose a reason for hiding this comment

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

Made a change to that comment. I am still not sure about wording the documentation for containerElement.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

At the very least, we should be consistent with the FlatButton and RaisedButton.
I think that we need the same PR for the next branch, we have a property for the same use case.

@@ -134,6 +134,13 @@ class RaisedButton extends Component {
*/
className: PropTypes.string,
/**
* An element like Link tag or a string can be used inside RaisedButton.
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should make the description generic, i.e. always the same in proptypes documentatino generator.
We have that one on the GridTitle that's a bit too verbose but more explicit:

     /**
     * Either a string used as tag name for the tile root element, or a ReactElement.
     * This is useful when you have, for example, a custom implementation of
     * a navigation link (that knows about your routes) and you want to use it as the primary tile action.
     * In case you pass a ReactElement, please ensure that it passes all props,
     * accepts styles overrides and render it's children.
     */

Copy link
Author

Choose a reason for hiding this comment

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

Updated it and made a PR for FlatButton.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation 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 Feb 21, 2017
@mbrookes
Copy link
Member

mbrookes commented Feb 21, 2017

Could we make the wording consistent across this, #6204, #6210, and any other components that support it?

@Shahrukh-Zindani
Copy link
Author

sure.

@oliviertassinari oliviertassinari changed the title [Docs] Add documentation for containerElement in RaisedButon [docs] Add documentation for containerElement in RaisedButon Feb 21, 2017
@Shahrukh-Zindani
Copy link
Author

It is consistent between FlatButton, RaisedButton and ListItem. Thanks @lourd for the PR on ListItem.

@oliviertassinari oliviertassinari 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 22, 2017
@Shahrukh-Zindani
Copy link
Author

@oliviertassinari Are you sure that containerElement is a prop in the Button tag in the next branch. I tried using it and it does not work nor do I see the prop being defined in the code anywhere.

@oliviertassinari
Copy link
Member

@Shahrukh-Zindani Do you want the answer or I let you dig in? (hint: the property name is different and more intuitive)

@Shahrukh-Zindani
Copy link
Author

Got it. Let me dig in.

@mbrookes mbrookes merged commit aa4db0e 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