-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix(module:result): show default icon based on status #9568
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9568 +/- ##
=======================================
Coverage 90.86% 90.86%
=======================================
Files 574 575 +1
Lines 23261 23270 +9
Branches 4639 4641 +2
=======================================
+ Hits 21135 21145 +10
- Misses 1393 1396 +3
+ Partials 733 729 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: HyperLife1119 <[email protected]>
Co-authored-by: HyperLife1119 <[email protected]>
HyperLife1119
left a comment
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.
@copilot Do not use contentChild to determine if customIconDirective exists; instead, use the fallback content of <ng-content select="[nz-result-icon]"></ng-content> to render the default icon.
Co-authored-by: HyperLife1119 <[email protected]>
Refactored to use Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@Laffery 可以看看这个 PR,可以的话还需要合并到 v20 吧 |
好的。还请将 PR 从 draft 状态改下 |
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.
Pull request overview
This PR fixes a bug where the nz-result component didn't display default icons when only nzStatus was set without providing a custom nzIcon. The fix uses ng-content fallback to render default status-based icons.
Key Changes:
- Added
defaultIcon()computed property to map status to default icon names - Modified template to include fallback content in ng-content that renders the default icon
- Added comprehensive test coverage for all four status types (success, error, info, warning)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| components/result/result.component.ts | Added defaultIcon() computed property and updated template to use ng-content fallback for rendering default icons based on status |
| components/result/result.spec.ts | Added new test component and test suite with 4 test cases to verify default icon rendering for each status type |
Laffery
left a comment
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.
LGTM
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: HyperLife1119 <[email protected]>
Co-authored-by: Copilot <[email protected]>
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?
When using
nz-resultwith onlynzStatusset (e.g.,nzStatus="success"), no icon is displayed. Theicon()computed property only checkednzIconinput and returnedundefinedwhen not provided.Issue Number: #9015
What is the new behavior?
Default icons now display based on
nzStatuswhennzIconis not provided:success→ check-circleerror→ close-circleinfo→ exclamation-circlewarning→ warningContent projection via
[nz-result-icon]directive still works when provided.Changes:
ng-contentfallback content to render the default icon when no custom[nz-result-icon]is projecteddefaultIcon()computed property to get the icon fromIconMapbased onnzStatusDoes this PR introduce a breaking change?
Other information
Added 4 test cases verifying default icon display for each status type.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.