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

[CardHeader] Make avatar prop optional #1879

Closed
wants to merge 1 commit into from

Conversation

sjstebbins
Copy link
Contributor

CardHeader shouldn't be required to have an Avatar. The avatar should be blank if no prop is assigned.

CardHeader shouldn't be required to have an Avatar. The avatar should be blank if no prop is assigned.
@sjstebbins sjstebbins changed the title Make avatar prop optional for CardHeader [CardHeader] make avatar prop optional Oct 15, 2015
@sjstebbins sjstebbins changed the title [CardHeader] make avatar prop optional [CardHeader] Make avatar prop optional Oct 15, 2015
@@ -94,14 +94,15 @@ const CardHeader = React.createClass({
let titleStyle = this.prepareStyles(styles.title, this.props.titleStyle);
let subtitleStyle = this.prepareStyles(styles.subtitle, this.props.subtitleStyle);

let avatar = this.props.avatar;
let avatar = this.props.avatar || null;
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 it's better to use the getDefaultProps

Choose a reason for hiding this comment

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

What if you don't set a default at all and just check on line 102 for
else if (typeof this.props.avatar === 'string') {

Seems to express the intention of the if-statement better, imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

@oliviertassinari did you have more feedback for @thataustin ?

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 26, 2015
@oliviertassinari
Copy link
Member

This is resolved by #2397.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants