-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Divider Component updated to v1 API #268
Conversation
jhchill666
commented
Jun 22, 2016
- Divider Component updated to v1 API
- Documents updated to reflect
- Divider-test added to suite
- Divider Component updated to v1 API - Documents updated to relect - Divider-test added to suite
Current coverage is 85.55%
|
|
||
return ( | ||
<DividerComponent className={classes} {...rest}> | ||
{_children} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the children directly here? Not sure why the fragment is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. I'd just whittled down the Label version, and not considered createFragment
was now redundant
Wow, nice work! Thanks. Just the one question about the fragment then we can merge this guy. |
import * as common from 'test/specs/commonTests' | ||
|
||
describe('Divider', () => { | ||
common.isConformant(Divider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add common.rendersChildren and common.hasUIClassName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also common tests for the classNames, reference https://github.com/TechnologyAdvice/stardust/blob/master/test/specs/elements/Label/Label-test.js
Ok there you have it @levithomason |
👍 |
I've added #269 to track the remaining components needing updated. I've also added a section to the README outlining "How can I help?". Your PR was much appreciated, would love to see more! |