-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(module:select): support custom template in select component #3071
feat(module:select): support custom template in select component #3071
Conversation
I also tried to add a
However, |
Codecov Report
@@ Coverage Diff @@
## master #3071 +/- ##
==========================================
+ Coverage 95.45% 95.47% +0.01%
==========================================
Files 685 685
Lines 14042 14077 +35
Branches 1865 1874 +9
==========================================
+ Hits 13404 13440 +36
+ Misses 231 226 -5
- Partials 407 411 +4
Continue to review full report at Codecov.
|
Deploy preview for ng-zorro-master ready! Built with commit 761c176 |
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 @ddvkid
thanks a lot for your pr
plz make sure your pr meet the commit guideline https://github.com/NG-ZORRO/ng-zorro-antd/blob/master/CONTRIBUTING.md#commit
Thanks for your comments, can you be more specific what I need to change? how about merging all the commits into one with more meaningful message? |
https://github.com/NG-ZORRO/ng-zorro-antd/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Amerged |
c1a1f06
to
c5f53d5
Compare
@vthinkxie any idea why the checks failed, and is the new commit format okay? |
@vthinkxie is this still going to be reviewed? |
@ddvkid commit message is ok, plz check your test and rebase onto the latest master |
541b371
to
6f583b1
Compare
@ddvkid Hello! Please rebase to the master branch. |
f90d912
to
a0241fa
Compare
@vthinkxie @wendzhue the tests have been fixed, can you please have a review? |
@vthinkxie updates? |
6e0171a
to
854bce2
Compare
templateUrl: './nz-select-top-control.component.html' | ||
templateUrl: './nz-select-top-control.component.html', | ||
host: { | ||
'[class.ant-select-selection--single]': 'nzSelectService.isSingleMode', |
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.
there is no need to add any className here
https://github.com/NG-ZORRO/ng-zorro-antd/blob/master/components/select/nz-select.component.html#L18
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.
Yeah this is because I moved these into the component's host https://github.com/NG-ZORRO/ng-zorro-antd/pull/3071/files#diff-290abbcec228b4006c68b790f2878cf5L18
I can change it back in order to be consistent.
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.
https://github.com/NG-ZORRO/ng-zorro-antd/pull/3071/files#diff-df1d27763e040098623dc15b1f22c3f8R40
no need to add any className or style here
templateUrl: './nz-select-top-control.component.html' | ||
templateUrl: './nz-select-top-control.component.html', | ||
host: { | ||
'[style.cursor]': 'nzShowSearch ? "inherit" : "pointer"' |
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.
any reason add this line?
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.
Hmm.. I can't recall it's been too long... let me try.
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 couldn't replicate the issue I ran into... it might be fixed by other updates, so I just removed 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.
Everything looks great now, approved
Thanks a lot for your contribution!
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: #2946
What is the new behavior?
Does this PR introduce a breaking change?
Other information