feat(icon): Add support to svg symbol icon set #11162

cobisimo · 2018-03-13T01:01:38Z

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Currently icon sets with symbols are not supported. Icon don't display as symbols are not rendered.

Issue Number: #8689

What is the new behavior?

This is kind of backport from material2 (angular 2 material design) project. You can check this:
https://github.com/angular/material2/blob/54252d5c1be882e6ffaec0f72516363f66b9de58/src/lib/icon/icon-registry.ts#L410
If detect symbol element iinstead svg within icon set convert it to svg.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

googlebot · 2018-03-13T01:01:42Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-03-13T02:07:07Z

CLAs look good, thanks!

Splaktar · 2018-03-23T06:40:51Z

Thank you for your contribution. Please squash your commits and update the commit message to follow the commit message guidelines. This is important for your changes to properly show up in the changelog.

Splaktar · 2018-03-23T06:49:45Z

The description of the current behavior here and the related fix in this PR don't seem to map directly to #8689. It does seem related, but it doesn't seem to be the same exact issue.

Can you please provide a CodePen that demonstrates the issue being solved here? The code seems to make sense, but it's not clear that there is an issue without a demo that reproduces the issue.

Splaktar

Can you please add a test that verifies this fix? We would hate to have a regression to this functionality in the future due to a missing test.

cobisimo · 2018-03-28T19:37:14Z

This PR is not direct fix for #8689 issue, but related to it. It is directly related to this closed issue #1514 (I didn't wont to do PR against closed ticket, but can change if you think it is better).
The issue here is that md-svg-icon use unique svg sprite <svg> packed within <svg>. There are multiple svg icon sprite generators like svg-sprite-loader, webpack-svgstore-plugin, gulp-svg-sprites ... They all pack icons as <symbol> within <svg>. If we try to use such icon pack it will not work, as <symbol> get embeded in <md-icon> instead <svg> and symbols are not renderable. This fix make addition to present setup to detect if source icon map use symbols and transfer them to svgs on the fly while creating icon cache.
Usual use of symbols to render icons is using <use xlink:href="#icon-name"></use>. This approach is good as it don't require icon cache, but switch to this require more changes to md-icon and have issue working on some browsers. As I said I didn't apply this logical way, I make a mix, using symbol within present setup, the similar way as it is done in material2.

Splaktar · 2018-03-28T19:53:47Z

OK, thank you for the link to #1514. That's good to know.

We'll certainly need a unit test to cover this behavior.

It might be worth looking into adding a demo for this in the icon demos as well. If the demo gets too complicated or you have issues with it, please let me know.

cobisimo · 2018-03-30T23:10:32Z

@Splaktar I added unit tests and demo with 2 additional icons to Demos -> Icons -> Svg Icon Sets block. Also commits should be squashed.

Splaktar · 2018-04-05T02:53:25Z

@cobisimo thanks for adding the tests and demos. I'll take a look.

It looks like the commits didn't get squashed properly. You'll need to force push your branch to GitHub after squashing in order for things to work properly.

Splaktar

Thanks for adding the tests, they look good. The demo is very useful as well. I had a question about the spinner icon and a couple minor comments.

Splaktar · 2018-04-05T02:56:07Z

src/components/icon/demoSvgIconSets/index.html

+      <md-icon md-svg-icon="social:cake"    style="color: #f00;width:60px;height:60px;" aria-label="Cake Icon"></md-icon>
+      <md-icon md-svg-icon="social:people"  style="color: #00F;" class="s48"            aria-label="People Icon"></md-icon>
+      <md-icon md-svg-icon="symbol:spinner" style="color: #f00;width:60px;height:60px;" aria-label="Spinner Icon"></md-icon>
+      <md-icon md-svg-icon="symbol:angular" class="s48"                                   aria-label="Angular Icon"></md-icon>


Minor nit, but it looks like the white space isn't lined up for aria-label in this last icon.

Splaktar · 2018-04-05T03:01:57Z

src/components/icon/js/iconService.js

+      if (viewbox) el.setAttribute('viewBox', viewbox);
+    }
+
+    if (el && el.tagName.toLowerCase() != 'svg') {


Please use !== here.

Splaktar · 2018-04-05T03:07:02Z

docs/app/img/icons/sets/symbol-icons.svg

@@ -0,0 +1,4 @@
+<svg>
+<symbol id="spinner" viewBox="0 0 44 44"><path d="M5.386 19.817c-.584 4.44.595 8.84 3.32 12.39 5.628 7.333 16.172 8.722 23.504 3.095 7.332-5.628 8.72-16.173 3.093-23.504-3.57-4.65-9.406-7.12-15.238-6.447-.264.028-.51-.16-.54-.428-.03-.267.16-.51.43-.54 6.166-.707 12.345 1.902 16.12 6.822 5.956 7.76 4.488 18.917-3.27 24.87-3.215 2.467-7.01 3.66-10.78 3.66-5.33 0-10.604-2.385-14.092-6.932C5.048 29.042 3.8 24.386 4.42 19.69c.595-4.532 2.86-8.578 6.392-11.447H5.57c-.27 0-.488-.22-.488-.488 0-.27.218-.488.487-.488h6.678c.27 0 .488.218.488.488v6.68c0 .27-.218.487-.488.487s-.488-.217-.488-.488V8.737c-3.53 2.724-5.792 6.657-6.374 11.08z"><animateTransform attributeName="transform" type="rotate" from="0 22 22" to="360 22 22" dur="3s" repeatCount="indefinite"/></path></symbol>


I like the AngularJS logo SVG! Where does this spinner come from? Did you create it or do you have a link to the related license?

I was using icon from webpack-svgstore-plugin. I change it now to material sync icon: https://material.io/icons/#ic_sync to not guess about licence.

Excellent, thank you.

Splaktar · 2018-04-05T03:11:01Z

Oh, one more thing, in your squashed commit, please use feat(icon): instead of feature(icon): so that our changelog will pick it up correctly.

Add support for SVG icon sets with symbol elements. SVG icon sprite with symbols is most commonly produced by: svg-sprite-loader, webpack-svgstore-plugin, gulp-svg-sprites ... This may consider also as backport, as angular material2 project support this kind of sets. This change is related to angular#8689 issue and directly related to closed issue angular#1514

Splaktar

LGTM

Add support for SVG icon sets with symbol elements. SVG icon sprite with symbols is most commonly produced by: svg-sprite-loader, webpack-svgstore-plugin, gulp-svg-sprites ... This may consider also as backport, as angular material2 project support this kind of sets. This change is related to angular#8689 issue and directly related to closed issue angular#1514

Add support for SVG icon sets with symbol elements. SVG icon sprite with symbols is most commonly produced by: svg-sprite-loader, webpack-svgstore-plugin, gulp-svg-sprites ... This may consider also as backport, as angular material2 project support this kind of sets. This change is related to #8689 issue and directly related to closed issue #1514

Splaktar · 2018-12-17T02:27:06Z

src/components/icon/js/iconService.js

+    // If the node is a <symbol>, it won't be rendered so we have to convert it into <svg>.
+    if (el && el.tagName.toLowerCase() === 'symbol') {
+      var viewbox = el.getAttribute('viewBox');
+      el = angular.element('<svg xmlns="http://www.w3.org/2000/svg">').html(el.innerHTML)[0];


I just wanted to give a heads up that this caused a regression / exception as innerHTML is not supported for SVG elements in IE11. Investigation happening here.

innerHTML is not supported on svg or symbol elements in IE11 use XMLSerializer instead of innerHTML when innerHTML is not defined Fixes #11543. Relates to #11342. Relates to #11162.

innerHTML is not supported on svg or symbol elements in IE11 use XMLSerializer instead of innerHTML when innerHTML is not defined add a test for empty SVGs to match a g3 test Fixes #11543. Relates to #11342. Relates to #11162.

googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Mar 13, 2018

googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Mar 13, 2018

Splaktar added the needs: squash commits label Mar 23, 2018

Splaktar self-assigned this Mar 23, 2018

Splaktar requested review from Splaktar and devversion March 23, 2018 06:41

Splaktar added the needs: unit tests This PR needs unit tests to cover the changes being proposed label Mar 23, 2018

Splaktar added the needs: demo A CodePen demo or GitHub repository is needed to demonstrate the reproduction of the issue label Mar 23, 2018

Splaktar suggested changes Mar 23, 2018

View reviewed changes

Splaktar suggested changes Apr 5, 2018

View reviewed changes

Splaktar added this to the 1.1.9 milestone Apr 5, 2018

Splaktar added the type: enhancement label Apr 5, 2018

Splaktar changed the title ~~feature(icon): Add support to svg symbol icon set~~ feat(icon): Add support to svg symbol icon set Apr 5, 2018

cobisimo force-pushed the svg-symbol-icon branch from f2c6d50 to d85f596 Compare April 10, 2018 13:37

Splaktar approved these changes Apr 10, 2018

View reviewed changes

Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: squash commits labels Apr 10, 2018

Splaktar assigned tinayuangao and mmalerba and unassigned Splaktar and tinayuangao Apr 11, 2018

mmalerba merged commit 5111f9d into angular:master Apr 17, 2018

Splaktar mentioned this pull request Dec 17, 2018

icon: unable to get property 'replace' of undefined or null reference at transformClone #11543

Closed

Splaktar reviewed Dec 17, 2018

View reviewed changes

Splaktar mentioned this pull request Dec 17, 2018

fix(icon): exceptions thrown by IE11 #11545

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(icon): Add support to svg symbol icon set #11162

feat(icon): Add support to svg symbol icon set #11162

cobisimo commented Mar 13, 2018 •

edited

Loading

googlebot commented Mar 13, 2018

googlebot commented Mar 13, 2018

Splaktar commented Mar 23, 2018

Splaktar commented Mar 23, 2018

Splaktar left a comment

cobisimo commented Mar 28, 2018

Splaktar commented Mar 28, 2018

cobisimo commented Mar 30, 2018

Splaktar commented Apr 5, 2018

Splaktar left a comment

Splaktar Apr 5, 2018

Splaktar Apr 5, 2018

Splaktar Apr 5, 2018

cobisimo Apr 10, 2018

Splaktar Apr 10, 2018

Splaktar commented Apr 5, 2018

Splaktar left a comment

Splaktar Dec 17, 2018

		@@ -0,0 +1,4 @@
		<svg>
		<symbol id="spinner" viewBox="0 0 44 44"><path d="M5.386 19.817c-.584 4.44.595 8.84 3.32 12.39 5.628 7.333 16.172 8.722 23.504 3.095 7.332-5.628 8.72-16.173 3.093-23.504-3.57-4.65-9.406-7.12-15.238-6.447-.264.028-.51-.16-.54-.428-.03-.267.16-.51.43-.54 6.166-.707 12.345 1.902 16.12 6.822 5.956 7.76 4.488 18.917-3.27 24.87-3.215 2.467-7.01 3.66-10.78 3.66-5.33 0-10.604-2.385-14.092-6.932C5.048 29.042 3.8 24.386 4.42 19.69c.595-4.532 2.86-8.578 6.392-11.447H5.57c-.27 0-.488-.22-.488-.488 0-.27.218-.488.487-.488h6.678c.27 0 .488.218.488.488v6.68c0 .27-.218.487-.488.487s-.488-.217-.488-.488V8.737c-3.53 2.724-5.792 6.657-6.374 11.08z"><animateTransform attributeName="transform" type="rotate" from="0 22 22" to="360 22 22" dur="3s" repeatCount="indefinite"/></path></symbol>

feat(icon): Add support to svg symbol icon set #11162

feat(icon): Add support to svg symbol icon set #11162

Conversation

cobisimo commented Mar 13, 2018 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Mar 13, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Mar 13, 2018

Splaktar commented Mar 23, 2018

Splaktar commented Mar 23, 2018

Splaktar left a comment

Choose a reason for hiding this comment

cobisimo commented Mar 28, 2018

Splaktar commented Mar 28, 2018

cobisimo commented Mar 30, 2018

Splaktar commented Apr 5, 2018

Splaktar left a comment

Choose a reason for hiding this comment

Splaktar Apr 5, 2018

Choose a reason for hiding this comment

Splaktar Apr 5, 2018

Choose a reason for hiding this comment

Splaktar Apr 5, 2018

Choose a reason for hiding this comment

cobisimo Apr 10, 2018

Choose a reason for hiding this comment

Splaktar Apr 10, 2018

Choose a reason for hiding this comment

Splaktar commented Apr 5, 2018

Splaktar left a comment

Choose a reason for hiding this comment

Splaktar Dec 17, 2018

Choose a reason for hiding this comment

cobisimo commented Mar 13, 2018 •

edited

Loading