-
Notifications
You must be signed in to change notification settings - Fork 1
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
added tests; refactor some code. #12
Conversation
1 similar comment
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.
Why did the coverage go down if we added more tests?
@@ -16,7 +16,8 @@ import SelectOption from './SelectOption'; | |||
import OutsideClick from 'ship-components-outsideclick'; | |||
import HighlightClick from 'ship-components-highlight-click'; | |||
|
|||
import dispatch from './dispatch'; | |||
import dispatch from './lib/dispatch'; |
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.
Hey, did you test this in any of the apps? I'm not sure the regex we use in webpack will babelify files in sub folders of the apps.
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.
I did not. I just tested the examples page which seems to be working.
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.
Verified to be working in OneSony against this webpack loader regex: /ship-components-.*\/src/
Should be ok to merge up now.
@@ -70,7 +71,7 @@ export default class Select extends React.Component { | |||
*/ | |||
componentDidUpdate(prevProps, prevState) { | |||
if (this.props.options.length > 5 && this.refs.selected && prevState.active === false && this.state.active === true) { | |||
this.refs.selected.scrollIntoView(); | |||
scrollIntoView(ReactDOM.findDOMNode(this.refs.selected)); |
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.
Do we need ReactDOM here?
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.
Yes, in this case "scrollIntoView" was a method of the SelectOption Component class. So in short, when I separated scrollIntoView into a lib folder it became a static function that requires a DOM node.
src/__tests__/Select-spec.jsx
Outdated
); | ||
|
||
expect(() => TestUtils.renderIntoDocument(element)) | ||
.not.toThrow(); | ||
}); | ||
|
||
it('should support custom css classes', () => { | ||
it('defines refs to input and list nodes', () => { |
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.
What does testing if the refs get set do? What's the functionality here you are testing?
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.
Good point. Other tests that assert functionality should implicitly handle this case.
No idea why coveralls rating decreased. I moved a couple functions into their own files under |
No description provided.