-
-
Notifications
You must be signed in to change notification settings - Fork 112
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
test: adding test to rating component. #375
Conversation
const component = mount( | ||
<Rating value="2" />, | ||
); | ||
component.find('input[value=3]').simulate('mouseleave'); |
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 think it is better first make:
component.find('input[value=3]').simulate('mouseover');
and:
expect(component.state().value).toBe('3');
then:
component.find('input[value=3]').simulate('mouseleave');
expect(component.state().value).toBe('2');
Because if you only simulate mouseleave the state never was set to 3 and always was 2.
The goal of this test is that the state change again to the value passed as prop.
value="1" | ||
name="name-1" | ||
onChange={() => {}} | ||
filled />, |
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.
filled is not a prop that you pass to RatingItems
filled: true, | ||
}); | ||
}); | ||
}); |
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.
add test:
should render the right amount of Star components
In this case 5. But not couple the name of the test with 5 because if we change this in the future then the test name need to change too.
component.find('input').simulate('change'); | ||
expect(component.find('StarFill').exists()).toBe(true); | ||
}); | ||
}); |
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.
and when filled is false? Should render the other icon. Test it.
And add test for the right value passed for AssistiveText
src/components/Rating/index.js
Outdated
@@ -39,7 +39,7 @@ export default class Rating extends Component { | |||
const { value } = this.state; | |||
return ( | |||
<fieldset | |||
onMouseEnter={this.handleOnHover} | |||
onMouseOver={this.handleOnHover} | |||
onMouseLeave={this.handleOnLeave} | |||
name={name} |
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.
name need to be passed to RatingItems instead of fieldset
src/components/Rating/index.js
Outdated
@@ -63,7 +64,7 @@ Rating.propTypes = { | |||
}; | |||
|
|||
Rating.defaultProps = { | |||
value: '', | |||
value: '0', |
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 think here you can put undefined since 0 is not a right value
* @method | ||
*/ | ||
click() { | ||
$(this.rootElement).$('input').click(); |
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 not input[type="radio"] here?
Consistency or all with input or with input[type="radio"] but all with the same.
src/components/Rating/ratingItems.js
Outdated
@@ -12,12 +12,14 @@ export default function RatingItems(props) { | |||
return Array(5).fill(0).map((item, index) => { | |||
const key = `star-${index}`; | |||
const filled = index < value; | |||
const isChecked = index === value; |
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.
here I think should be:
const isChecked = index + 1 === value;
since value is base 1 and index is base 0.
I mind if min value is 1 then the item with index 0 in never checked.
src/components/Rating/smallStar.svg
Outdated
@@ -0,0 +1,11 @@ | |||
<svg width="6px" height="6px" viewBox="0 0 19 18" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlnsXlink="http://www.w3.org/1999/xlink"> |
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.
since this is part of the example it is not part of the component then put it in the assets/image folder
Pull Request Test Coverage Report for Build 180
💛 - Coveralls |
No description provided.