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

[Doc] Migrate Badge to the new format #2489

Merged
merged 2 commits into from
Dec 13, 2015
Merged

[Doc] Migrate Badge to the new format #2489

merged 2 commits into from
Dec 13, 2015

Conversation

oliviertassinari
Copy link
Member

There is two commits here. One for converting the Badge component.
The second is for using the stateless approach for other example. However, I have added the return keyword.
I feel like it's clearer like this. Any though? @subjectix @newoga.
Maybe I should just remove this return keyword.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation PR: Needs Review labels Dec 11, 2015
@alitaheri
Copy link
Member

There is two commits here. One for converting the Badge component.

yes, it's better this way, but two separate PRs would have been ideal, since the changes you are going to make to this PRs are gonna have to be squashed down to the second one, even if they are related to the Migrate Badge to the new format commit. but don't sweat it so much. I'd say just squash down to one and add a long note to the one commit you'll create.

Maybe I should just remove this return keyword.

I think it's better to keep the examples short for the sake of brevity. So, it's better without the return statement. But the source code will suffer from this pattern.

I will have a look at your PR thanks a lot 👍

@newoga
Copy link
Contributor

newoga commented Dec 12, 2015

👍 I'm on board with writing the examples without return as well. It's not often this pattern is even applicable because you probably more often than not need to compute something from props/context first.

@alitaheri
Copy link
Member

@oliviertassinari I would suggest breaking down the example into 4 different examples, this is a bit complex to understand. but 4 tiny examples (each can have like less than 10 lines) is a lot more comprehensive imo.

@oliviertassinari
Copy link
Member Author

@subjectix I have split the examples in two.
One with numbers as a badgeContent and one with react elements as a badgeContent.

@oliviertassinari oliviertassinari changed the title Docs codegen badge [Doc] Migrate Badge to the new format Dec 13, 2015
secondary: React.PropTypes.bool,

/**
* Override the inline-styles of the root element.
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm proposing this standard description. I don't think that we need to say that it's for the Badge component.
I have done the same for the className.

Copy link
Member

Choose a reason for hiding this comment

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

I like your approach. be sure to remind me when I make this mistake again 👍 👍

@alitaheri
Copy link
Member

It's great. nice work 👍 merge when ready (with both commits 😬)

@oliviertassinari
Copy link
Member Author

@subjectix Thanks, I'm merging.

oliviertassinari added a commit that referenced this pull request Dec 13, 2015
@oliviertassinari oliviertassinari merged commit f243857 into mui:master Dec 13, 2015
@oliviertassinari oliviertassinari deleted the docs-codegen-badge branch December 13, 2015 23:41
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.

3 participants