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

Label component #215

Merged
merged 5 commits into from
Apr 4, 2016
Merged

Label component #215

merged 5 commits into from
Apr 4, 2016

Conversation

levithomason
Copy link
Member

Adds <Label /> component and tests. A separate PR will add docs.

@levithomason levithomason force-pushed the feature/label branch 23 times, most recently from c20bb3e to 294c2ee Compare April 2, 2016 23:32
@@ -37,7 +37,7 @@ export default class TryStardust extends Component {
- [x] Icon
- [x] Image
- [x] Input
- [ ] Label
- [x] Label
Copy link
Member Author

Choose a reason for hiding this comment

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

Mark it off in the support section of the README.

"release:major": "ta-script npm/release.sh major",
"release:minor": "ta-script npm/release.sh minor",
"release:patch": "ta-script npm/release.sh patch",
"start": "gulp",
"start": "npm run docs",
Copy link

Choose a reason for hiding this comment

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

It seems it may be clearer to remove the docs script and call npm run gulp here. Is there a reason one may also want to run npm run docs separately?

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 think npm start should always start the project without a need to understand the underlying implementation. Also, if we drop gulp (which we hope to do), npm start should still start the project. If we tie the start script to gulp, then there is unnecessary coupling that would require updates to the README and dev workflow since they'll need to run a new command.

npm start might not always run the doc site, it will always "start the project". There is an npm run docs that will always run the doc site.

This is my rationale at least. Open to change if there is pretty good consensus that thinks otherwise.

@@ -0,0 +1,40 @@
import _ from 'lodash'
import { Children } from 'react'
Copy link
Contributor

Choose a reason for hiding this comment

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

Was curious as to the utilities associated with React.Children found documentation here and posting here for other's convenience.

@ghost
Copy link

ghost commented Apr 4, 2016

🍶

@athurman
Copy link
Contributor

athurman commented Apr 4, 2016

LGTM 🐙


/** Primary text of the label, same as children. */
text: PropTypes.node,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you found a lot of benefit in making the label a pure function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, it is actually more LOC and limits us from using life cycle methods. However, they will take advantage of extra optimization in future versions of React. So, we're getting on the bandwagon now.

@eanplatter
Copy link
Contributor

Looks good: 🍂

@levithomason levithomason merged commit 66bfeff into master Apr 4, 2016
@levithomason levithomason deleted the feature/label branch April 4, 2016 16:42
@levithomason levithomason mentioned this pull request Apr 4, 2016
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.

3 participants