Incremental PR towards #347 #462
Conversation
|
Hey! Changelogs info seems to be missing or might be in incorrect format. |
|
@kolharsam Can you add failing test case, which this PR fixes. |
|
Looks good. Approved. |
|
@kolharsam Looks like some specs are failing after this merge. Can you check that. |
|
Merged. Working. |
| @@ -56,6 +56,33 @@ describe('NumberFormat as input', () => { | |||
| expect(wrapper.find('input').instance().value).toEqual('$2,456,981'); | |||
| }); | |||
|
|
|||
| it('should load the default value when initial value is null', () => { | |||
| const wrapper = mount(<NumberFormat value={null} defaultValue={89} />); | |||
| expect(wrapper.state().value).toEqual('89'); | |||
There was a problem hiding this comment.
We should never check for state value. As it's implementation details. Instead we should test for the end side-effect.
expect(wrapper.find('input').instance().value).toEqual('89');| expect(wrapper.state().value).toEqual('89'); | ||
| }); | ||
|
|
||
| it('should load the prevous valid value if the state is changed to null', () => { |
There was a problem hiding this comment.
Typo on the statement.
should load the previous valid value
| const domInput = input.instance(); | ||
|
|
||
| expect(domInput.value).toEqual('90'); | ||
| wrapper.setState({testState: null}); |
There was a problem hiding this comment.
This spec could be simplified you just mount <NumberFormat value={this.state.testState} /> and then change the props through setProps.
Also FYI. Try avoiding interacting with states in specs. You shouldn't depend much on implementation detail.
|
@kolharsam The test is still failing when I am trying to run the test. Also, missed reviewing specs earlier. Added some comments there. |
|
@kolharsam Can you try pulling code from origin and rerun the test. |
|
@s-yadav thanks for the review remarks. Will be making a new PR to fix this ASAP |
This PR is an incremental PR against #347 based on this comment