feat(icon): allow SVG icons to be registered from strings #10757
Conversation
googlebot
added
the
cla: yes
src/lib/icon/icon-registry.ts
Outdated
| * @param iconName Name under which the icon should be registered. | ||
| * @param template SVG source of the icon. | ||
| */ | ||
| addSvgIconTemplateInNamespace(namespace: string, iconName: string, template: SafeHtml): this { |
There was a problem hiding this comment.
@jelbourn I'm not a fan of having to have two sets of methods. Ideally the signature of addSvgIconInNamespace would be something like addSvgIconInNamespace(namespace: string, iconName: string, data: SafeResourceUrl | SafeHtml), however there's no clean way of detecting which type of resource was used, because SafeResourceUrl and SafeHtml are interfaces.
There was a problem hiding this comment.
Yeah, would be better, but ultimately doesn't bother me that much.
crisbeto
force-pushed
the
3132/icon-from-svg-string
branch
2 times, most recently
from
b27933b
to
eb5bb3f
Compare
src/lib/icon/icon-registry.ts
Outdated
| constructor(url: SafeResourceUrl); | ||
| constructor(svgElement: SVGElement); | ||
| constructor(data: SafeResourceUrl | SVGElement) { | ||
| if (data instanceof SVGElement) { |
There was a problem hiding this comment.
Will referencing SVGElement cause any issues on platform-server?
There was a problem hiding this comment.
Yep, looks like it does. I've switched it to a hackier approach that should work everywhere.
src/lib/icon/icon-registry.ts
Outdated
| * @param iconName Name under which the icon should be registered. | ||
| * @param template SVG source of the icon. | ||
| */ | ||
| addSvgIconTemplate(iconName: string, template: SafeHtml): this { |
There was a problem hiding this comment.
What do you think about addSvgIconLiteral or addSvgIconFromHtmlString? Template isn't really the right term because the string is totally static / non-templated
There was a problem hiding this comment.
I like "literal", I wanted to avoid having having HTML in there because SVG is an XML dialect rather an HTML one.
src/lib/icon/icon-registry.ts
Outdated
| * @param iconName Name under which the icon should be registered. | ||
| * @param template SVG source of the icon. | ||
| */ | ||
| addSvgIconTemplateInNamespace(namespace: string, iconName: string, template: SafeHtml): this { |
There was a problem hiding this comment.
Yeah, would be better, but ultimately doesn't bother me that much.
Expands the icon registry to allow for icons and icon sets to be registered using a trusted HTML string. Fixes angular#3132.
crisbeto
force-pushed
the
3132/icon-from-svg-string
branch
from
eb5bb3f
to
bad81e5
Compare
|
Addressed the feedback @jelbourn. |
jelbourn
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Expands the icon registry to allow for icons and icon sets to be registered using a trusted HTML string.
Fixes #3132.