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

Feature/add sd item #65

Merged
merged 14 commits into from
Oct 20, 2015
8 changes: 8 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ import ModalFooter from 'src/modules/Modal/ModalFooter';
import ModalHeader from 'src/modules/Modal/ModalHeader';
import Dropdown from 'src/modules/Dropdown/Dropdown';

// Views
import Item from 'src/views/Items/Item';
import Items from 'src/views/Items/Items';

export default {
// Addons
Confirm,
Expand Down Expand Up @@ -64,4 +68,8 @@ export default {
ModalFooter,
ModalHeader,
Dropdown,

// Views
Item,
Items,
};
16 changes: 16 additions & 0 deletions src/utils/customPropTypes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import _ from 'lodash';

let customPropTypes = {
mutuallyExclusive: (exclusives, props, propName, componentName) => {
_.forEach(exclusives, exclusiveProp => {
if (props[propName] && props[exclusiveProp]) {
throw new Error(`\`${componentName}\` should only have one of type \`${propName}\` or \`${exclusiveProp}\`, ` +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to throw error as opposed to return because the new Error needed to attach itself to the prop, not the utility property mutuallyExclusive

`not both.`);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to compare the specific propType to the exclusive props passed in, I had to take advantage React's custom validation function found in this article - so we have to bind the exclusives to this function in order to access and take advantage of the additional params props propName and componentName.

This way, we have the current prop to compare the exclusives to.


return null;
});
},
};

export default customPropTypes;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new utility to be used with customPropType validation and checking.

44 changes: 44 additions & 0 deletions src/views/Items/Item.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import React, {Component, PropTypes} from 'react';
import classNames from 'classnames';
import customPropTypes from '../../utils/customPropTypes';

export default class Item extends Component {
static propTypes = {
children: PropTypes.node,
className: PropTypes.string,
description: customPropTypes.mutuallyExclusive.bind(null, ['children']),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Binding the exclusive props to the mutuallyExclusive utility function here.

extra: PropTypes.node,
heading: PropTypes.node.isRequired,
image: PropTypes.node,
meta: PropTypes.node,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pass in all "pieces" of the component as props to make sure that all content matches semantic's markup.


render() {
let heading = <div className='header'>{this.props.heading}</div>;
let meta = <div className='meta'>{this.props.meta}</div>;
let description = <div className='description'>{this.props.children || this.props.description}</div>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, will render children before the default property if they both exist on the component.

let extra = <div className='extra'>{this.props.extra}</div>;
let content = (
<div className='middle aligned content'>
Copy link
Member

Choose a reason for hiding this comment

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

Stardust should not add default style classes that are not configurable. Perhaps exposing a contentClass prop would make sense. We can then use defaultProps to set it to this value but the user could supply their own classes. Use classNames() to merge the content classes the same as we do for className, adding sd-item-content and content classes automatically.

EDIT fixed sd-* class name typo.

{this.props.heading && heading}
{this.props.meta && meta}
{this.props.children && description || this.props.description && description}
{this.props.extra && extra}
</div>
);
let hasContent = !!this.props.heading || !!this.props.meta || !!this.props.extra || !!this.props.children
|| !!this.props.description;

let classes = classNames(
'sd-item',
this.props.className,
'item',
);
return (
<div {...this.props} className={classes}>
Copy link
Member

Choose a reason for hiding this comment

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

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.

{this.props.image}
{hasContent && content}
</div>
);
}
}
23 changes: 23 additions & 0 deletions src/views/Items/Items.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import React, {Component, PropTypes} from 'react';
import classNames from 'classnames';

export default class Items extends Component {
static propTypes = {
children: PropTypes.node,
className: PropTypes.string,
};

render() {
let classes = classNames(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parent items div if more than one item needs to exist.

'sd-items',
'ui',
this.props.className,
'items',
);
return (
<div {...this.props} className={classes}>
{this.props.children}
</div>
);
}
}
10 changes: 10 additions & 0 deletions test/specs/views/Items/Item-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import React from 'react';
import {Item} from 'stardust';
import faker from 'faker';

describe('Item', () => {
it('has children', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Small semantic request here. This test isn't testing that Item has children but that Item renders children.

let child = faker.hacker.phrase();
render(<Item description='foo'>{child}</Item>).findText(child);
});
});