Feature/add sd item #65
Conversation
| static propTypes = { | ||
| children: PropTypes.node, | ||
| className: PropTypes.string, | ||
| description: PropTypes.node, |
There was a problem hiding this comment.
Need to add utility for adding warning if children exists.
athurman
force-pushed
the
feature/add-sd-item
branch
from
029fa46
to
41b6509
Compare
|
🍂 |
| }, | ||
| }; | ||
|
|
||
| export default customPropTypes; |
There was a problem hiding this comment.
new utility to be used with customPropType validation and checking.
| if (props[propName] && props[exclusiveProp]) { | ||
| throw new Error( | ||
| `\`${componentName}\` should only have one of type \`${propName}\` or \`${exclusiveProp}\` not both.` | ||
| ); |
There was a problem hiding this comment.
I didn't think of throwing error messages with ticks. nice!
|
🍂 |
| 'item', | ||
| ); | ||
| return ( | ||
| <div {...this.props} className={classes}> |
There was a problem hiding this comment.
Component props used for Stardust purposes should be removed from this.props before spreading. Only the user's props should be spread. Since this component takes in:
children
className
description
extra
heading
image
meta
We should remove those props before spreading them onto the Item. Otherwise, we'll be spreading components and other unintended things:
let props = _.clone(this.props);
delete props.children;
delete props.className;
delete props.description;
delete props.extra;
delete props.heading;
delete props.image;
delete props.meta;
// then
<div {...props} ...Thinking about this, I believe we should always remove static PropTypes props, except for static defaultProps before spreading. This way we only spread "extra" user defined props and defaultProps the user didn't specify. Linking #69 spreadable props util here for this reference.
For now, add the clone/delete step before spreading.
| import faker from 'faker'; | ||
|
|
||
| describe('Item', () => { | ||
| it('has children', () => { |
There was a problem hiding this comment.
Small semantic request here. This test isn't testing that Item has children but that Item renders children.
|
Good to merge after hitting the last to comments: Also, can close #68. Nice work on that, and bundling the 👍 ⚡ |
2 New SD components for the Semantic UI Item view:
EDIT
PR also addresses GH Issue #68