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

Use React 0.14's stateless components #95

Closed

Conversation

rolyatmax
Copy link
Contributor

Some spots where we could use React 0.14's stateless components.

depends on #79

@rolyatmax
Copy link
Contributor Author

@AlanFoster Let me know what you think of this.

@AlanFoster
Copy link
Member

I'm a fan of the newer notation; But we should be consistent with the rest of the code base. Particularly since this will make it harder for us to add propType and defaultPropType information in the future if we need to. Likewise newcomers to React may not fully appreciate the terseness of this PR change.

I'll close this PR for now, but we might re-visit it in the future!

@AlanFoster AlanFoster closed this Oct 28, 2015
@rolyatmax
Copy link
Contributor Author

Do you mean that to use React's stateless functions syntax, we would need to use only stateless components in order to be consistent?

Or do you mean we should be consistent with how we define stateless components?:

const Link = ({href, text}) => <a href={href}>{text}</a>;

// versus

function Link({href, text}) {
    return <a href={href}>{text}</a>;
}

Also, for what it's worth, you can add propType validation like so:

function MyComponent({children}) {
    return <div>{children}</div>;
}

MyComponent.propTypes = {
    children: React.PropTypes.node.isRequired
};

const Index = React.createClass({
getDefaultProps() {
return {};
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't want to move ahead with this PR, should we pull this change out and merge separately?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants