Handle SVG sprite #32
Conversation
There was a problem hiding this comment.
Thank you! This pull request looks promising, but it needs a few additional changes.
First, it will need modifications to svg-icon.component.ts to pass the symbolID parameter to the service.
Second, the rollup.config.js will need to have the additional rxjs operations added to it so the module can be built successfully for distribution.
Third, there are some code quirks I would like to see resolved that I will mark in the code at the line they occur.
Third, I am in the process of converting to HttpClient (see the ng5 branch), so you may wish to work from that branch or wait until I merge into master (sometime in December).
| import 'rxjs/add/observable/of'; | ||
| import 'rxjs/add/operator/do'; | ||
| import 'rxjs/add/operator/finally'; | ||
| import 'rxjs/add/operator/map'; | ||
| import 'rxjs/add/operator/share'; | ||
| import 'rxjs/add/operator/mergeMap'; | ||
| import 'rxjs/add/operator/delay'; |
There was a problem hiding this comment.
Add additional rxjs/operators to rollup.config.js.
|
|
||
| loadSvg(url:string, symbolID:string = ''): Observable<SVGElement> { | ||
| const orgUrl = url; | ||
| url += `#${symbolID}`; |
There was a problem hiding this comment.
This adds # to all Urls whether or not they are using sprite symbols.
I would prefer that the original Url is kept as url and the sprite or symbol Url is changed, so something like:
const symbolUrl = url + `#${symbol}`;
| @@ -19,28 +20,45 @@ export class SvgIconRegistryService { | |||
| constructor(private http:Http) { | |||
| } | |||
|
|
|||
| loadSvg(url:string): Observable<SVGElement> { | |||
|
|
|||
| loadSvg(url:string, symbolID:string = ''): Observable<SVGElement> { | |||
There was a problem hiding this comment.
Prefer symbol rather than symbolID.
| const svgElement = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); | ||
| const spriteElement: SVGSVGElement = svg.querySelector(`#${symbolID}`); | ||
| svgElement.setAttribute('width', spriteElement.viewBox.baseVal.width + 'px'); | ||
| svgElement.setAttribute('height', spriteElement.viewBox.baseVal.height + 'px'); |
There was a problem hiding this comment.
Setting width and height circumvents width/height styling with CSS. So, please remove these two lines of code.
If setting width and height is desirable, then maybe an additional parameter in the component to instruct setting the viewbox width and height as the size.
| const spriteElement: SVGSVGElement = svg.querySelector(`#${symbolID}`); | ||
| svgElement.setAttribute('width', spriteElement.viewBox.baseVal.width + 'px'); | ||
| svgElement.setAttribute('height', spriteElement.viewBox.baseVal.height + 'px'); | ||
| svgElement.setAttribute('viewBox', spriteElement.getAttribute('viewBox')); |
|
Also, unsure how |
| // Extract sprite | ||
| getSprite(svg: SVGElement, symbolID: string) { | ||
| const svgElement = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); | ||
| const spriteElement: SVGSVGElement = svg.querySelector(`#${symbolID}`); |
There was a problem hiding this comment.
Had difficulty getting this to build without explicit casting:
const spriteElement:SVGSVGElement = <SVGSVGElement>svg.querySelector(`#${symbolID}`);
|
Hello! How's it going? |
|
@katyapavlenko - the PR would need to be updated to resolve the conflicts. I do not have the time to do it myself. |
|
@czeckd Any change to bring this into a version? Loading SVG sprites would be a great addition. |
|
@fynnfeldpausch - No change, it would take some work to refactor to merge into master. At this point, my guess is that it would largely be a rewrite and I do not have the time to do it. |
Thanks for this great component. I use it to load a bunch of icons. To minimize HTTP requests, I made an SVG sprite. This PR handle that.
Any feedback are welcome.