-
Notifications
You must be signed in to change notification settings - Fork 4k
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(module:cascader): should not change on hover work #1991
Conversation
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.
After fixing this issue, you should run npm run test
to make sure you didn't break anything. Behavior has hanged in the test case named should change on hover work
.
Sorry, this is my fault, I will test it again. |
I checked it and found that there are some problems with this test case. testComponent.nzChangeOnSelect = true;
testComponent.nzExpandTrigger = 'hover';
...
dispatchMouseEvent(itemEl1, 'mouseenter');
...
// In this situation, testComponent values can be undefined.
expect(testComponent.values).toBeDefined();
expect(testComponent.values.length).toBe(1);
expect(testComponent.values[ 0 ]).toBe('zhejiang'); So, Can I modify it? |
@shuizhongxiong Sure, go ahead. |
Codecov Report
@@ Coverage Diff @@
## master #1991 +/- ##
=======================================
Coverage 96.03% 96.03%
=======================================
Files 470 470
Lines 11417 11417
Branches 1513 1513
=======================================
Hits 10964 10964
Misses 130 130
Partials 323 323
Continue to review full report at Codecov.
|
I modified the test case and then ran the ‘npm run test’ command. Except for the ‘nz-autocomplete’ component, all other test cases passed. But I am sure that I have not touched this component, what should I do? |
@shuizhongxiong You can just ignore that error. But you shouldn't have removed the test case entirely... You should modify it to make sure it's behavior is what you have changed to. You can change test.ts to test cascader component only, and revert test.ts when you are going to commit. If you are not familiar with Angular's test you should probably read the doc. Just take your time. |
Thank you for your suggestion. I read some documentation about Angular's test and then fixed it again. Please take some time to check it out. If there is any problem, I will change it again. |
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.
Thanks for your excellent work!
@hsuanxyz Please review this. I think we can merge it. |
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.
Hi thanks for your pr, please make sure your pr meet our guidelines.
https://github.com/NG-ZORRO/ng-zorro-antd/blob/master/CONTRIBUTING.md#commit
Your commit title is too long, please reduce it. and should prefix with module
@hsuanxyz Can this title be ok? |
You can use git rebase to update your initial commit message to meet our guideline and push -f. |
c88c243
to
08a2ca9
Compare
Is this ok? |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #1966
What is the new behavior?
Does this PR introduce a breaking change?
Other information