Skip to content
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:popconfirm): Support custom icon. #2445

Closed
wants to merge 3 commits into from

Conversation

WOOOFEI
Copy link

@WOOOFEI WOOOFEI commented Nov 14, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #2196

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@WOOOFEI
Copy link
Author

WOOOFEI commented Nov 14, 2018

My first PR, I would appreciate your guidance😊.

@codecov
Copy link

codecov bot commented Nov 14, 2018

Codecov Report

Merging #2445 into next will decrease coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #2445      +/-   ##
==========================================
- Coverage   95.84%   95.83%   -0.01%     
==========================================
  Files         487      487              
  Lines       12318    12322       +4     
  Branches     1663     1663              
==========================================
+ Hits        11806    11809       +3     
- Misses        152      153       +1     
  Partials      360      360
Impacted Files Coverage Δ
components/popconfirm/nz-popconfirm.component.ts 100% <100%> (ø) ⬆️
components/popconfirm/nz-popconfirm.directive.ts 98.11% <66.66%> (-1.89%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b8f9ea...a8d478a. Read the comment docs.

Copy link
Member

@wzhudev wzhudev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!

It's basically useful but here are some enhancements that should be made:

  • Add test case.
  • Should support TemplateRef. What if users want to use their own icons or two-tone icons?
  • Commit message should be in lower case and has no period .
  • In your commit message you should write close #2196 in the second line, so that issue would be closed automatically

@wzhudev
Copy link
Member

wzhudev commented Nov 14, 2018

Oh, and the docs.

@WOOOFEI
Copy link
Author

WOOOFEI commented Nov 14, 2018

Roger!Thank you for your guidance, I'll try again.

@vthinkxie
Copy link
Member

HI @WOOOFEI we are going to merge next to master now, plz submit this pr to master now, closed now.

@vthinkxie vthinkxie closed this Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants