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

Fix conformance tests #171

Merged
merged 18 commits into from
Mar 16, 2016
Merged

Fix conformance tests #171

merged 18 commits into from
Mar 16, 2016

Conversation

levithomason
Copy link
Member

Fixes a number of oversights and issues with the conformance tests. Also fixes code to conform to now working conformance tests.

@@ -190,28 +190,28 @@ All magic is noted in the documentation examples.

```jsx
<Form.Field className='inherit-this' />
// => <div className='sd-field inherit-this field>...
// => <div className='sd-form-field inherit-this field>...
Copy link
Member Author

Choose a reason for hiding this comment

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

Components were reorganized to sub components in #99:
Field => FormField.

Generated classNames should also follow suit:
className='sd-field' => className='sd-form-field'

Fixing conformance tests pointed this out.

children: PropTypes.node,
className: PropTypes.string,
icon: PropTypes.node,
image: PropTypes.node,
};
}
Copy link

Choose a reason for hiding this comment

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

I love not having the ; on static props. What makes this possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Babel reverting their ridiculous breaking change where they threw errors if you didn't have them ;)

I actually meant to leave all them in and i'll pull them on another PR. Will leave this here for now...

@ghost
Copy link

ghost commented Mar 16, 2016

Nothing glaring but the ☀️ in my face. ⛵ with comments considered.

@@ -65,7 +65,7 @@ export default (config) => {
progress: false,
stats: statConfig,
debug: true,
noInfo: true,
noInfo: false,
Copy link
Member

Choose a reason for hiding this comment

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

por que?

import _ from 'lodash'
import React, { Component, PropTypes } from 'react'
import classNames from 'classnames'
import cx from 'classnames'
Copy link
Member

Choose a reason for hiding this comment

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

Not relevant to this line... I wonder if it would make sense to make this a pure component and not re-render unless props change. Since it's such a simple (and common) element it might be a nice future feature (or option) to do prop checks to prevent unneeded re-renders.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely, I've been skipping all optimizations while we crank toward 1.0. This is a great point.

@dvdzkwsk
Copy link
Member

👻 w/ comments

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