angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.38k stars 6.75k forks source link

icon: Keep symbol viewBox value when register an iconSet #5488

Open ilaborie opened 7 years ago

ilaborie commented 7 years ago

Bug, feature request, or proposal:

Bug

What is the expected behavior?

When registering an SVG iconSet, every symbol may provide a custom viewBox attribute. This attribute should be keep in the newly SVG element created.

What is the current behavior?

The viewBox attribute is swallowed.

What are the steps to reproduce?

What is the use-case or motivation for changing an existing behavior?

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

At least since beta 7

Is there anything else we should know?

This is related to #5188 #2981 For SVG icon set with symbol, see https://css-tricks.com/svg-symbol-good-choice-icons/

jelbourn commented 7 years ago

I think this is captured by work on #5188. md-icon is specifically intended to cover the Material Design icon treatment; a cdk-svg directive would be able to be a lot more general.

choucry13 commented 6 years ago

hi, i don't see the fix of @ilaborie in the current MatIconRegistry ? The fix is current and I always need a override of MatIconRegistry with this fix.

ilaborie commented 6 years ago

I still do not understand why this 3-lines patch is not applied?

choucry13 commented 6 years ago

hi @jelbourn, any news please ?

Meliovation commented 6 years ago

I struggled with trying to use a sprite svg file exported from FlatIcon.com (svg file using inline elements) with the addSvgIconSetInNamespace function. I finally realized the viewBox attribute is NOT inserted into the output svg code with the mat-icon component. I'm glad I found this github issue to confirm.

I'm wondering if anyone has a solution? Without the viewBox values, the SVG cannot be rendered and scaled properly. Is it just not possible? I'll register the icons individually with addSvgIcon if I have to; just thought it would be so much more efficient to have one combined svg file. Advice?

jelbourn commented 6 years ago

Potentially revisiting this now

bsteffl commented 5 years ago

Do you support both <defs> and <symbol> in an svg icon set? Symbols seem to work with the exception of the viewbox missing, which causes the icons to not be centered. I'm hesitant to manually fix my entire sprite file since it was easily dynamically generated, unless you don't support <symbol>.

Humberd commented 5 years ago

Here is the fix I applied in my code by overriding _toSvgElement method exactly like @ilaborie did:

@NgModule({
  providers: [
    {
      provide: APP_INITIALIZER,
      deps: [MatIconRegistry],
      useFactory: preserveSvgViewBox,
      multi: true
    }
  ]
})
export class IconModule {
}

function preserveSvgViewBox(matIconRegistry: MatIconRegistry) {
  return () => {
    const oldToSvgElement = matIconRegistry['_toSvgElement'];

    matIconRegistry['_toSvgElement'] = (element: Element) => {
      const svg = oldToSvgElement.call(matIconRegistry, element);

      const viewBox = element.getAttribute('viewBox');
      if (viewBox !== null) {
        svg.setAttribute('viewBox', viewBox);
      }
      return svg;
    };
  };
}
elpupi commented 5 years ago

Wonderful @Humberd . It is working like a charm. What a bad documentation though. And hard to find some issues. A pity foor such a good framework.

devHand commented 5 years ago

@bsteffl , from material documentation on icon sets:

This is done by creating a single root tag that contains multiple nested tags in its section. Each of these nested tags is identified with an id attribute. This id is used as the name of the icon.

The problem is that icons services like icomoon.io creats icon sets with <symbol>. You can replace it to <svg>, but why?

Wykks commented 5 years ago

I think it's fixed in the latest version :

https://github.com/angular/components/blob/d6e7893691425bbf17f610754e37ced7793a8029/src/material/icon/icon-registry.ts#L530-L537