-
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
fix(module:icon): do not try to load SVG on the Node.js side since it will throw an error #7242
Conversation
This preview will be available after the AzureCI is passed. |
Codecov Report
@@ Coverage Diff @@
## master #7242 +/- ##
==========================================
- Coverage 91.72% 91.70% -0.02%
==========================================
Files 486 486
Lines 15918 15921 +3
Branches 2588 2588
==========================================
Hits 14601 14601
- Misses 1004 1007 +3
Partials 313 313
Continue to review full report at Codecov.
|
… will throw an error
components/icon/icon.directive.ts
Outdated
// this SVG, so we'll have an unnecessary HTTP request on the server-side. There's no sense to | ||
// call `_changeIcon()` when running the code on the Node.js side. | ||
if (!this.platform.isBrowser) { | ||
return; |
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.
Should support changing icon when using static loading. I would provide an API in @ant-design/icons-angular to turn dynamic loading off.
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.
Alright, I'll make the changes after that.
@arturovt I made some changes. Not sure if we should change |
Alright, I'm ok with changes, seems like I have to revert tests. |
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
This seems to still be an issue in 13.1.0? |
PR Checklist
PR Type
What is the current behavior?
Issue Number: #7240