Skip to content
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

feat(MatIconRegistry): "Wildcard-URL" support for svg icon sets #18607

Closed
philmtd opened this issue Feb 25, 2020 · 7 comments · Fixed by #21431
Closed

feat(MatIconRegistry): "Wildcard-URL" support for svg icon sets #18607

philmtd opened this issue Feb 25, 2020 · 7 comments · Fixed by #21431
Assignees
Labels
area: material/icon feature This issue represents a new feature or feature request rather than a bug or bug fix P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@philmtd
Copy link

philmtd commented Feb 25, 2020

Feature Description

The current approach of adding svg icons either results in loading a large svg sprite regardless how many icons are actually used (approach 2) or requires to register every icon individually (approach 1). Approach 1 is not viable because it requires developers to maintain a list of all used icons. Alternatively all icons could be added manually. This is an improvement over the sprite as users only download the used icons. The downside is that still all icon "metadata" has to be included in the app, even if 95% of icons are not used.

The ultimate goal would be to only include the icons in the build that are actually used. But it would be sufficient if the webapp (as seen from the user) only downloads the necessary icons (and metadata), even if the build includes all icons. Having the option to configure a directory for a svg icon set (where each icon resides as a single svg file) would solve this issue. An icon defined as <mat-icon svgIcon="foo"></mat-icon> would look up the icon foo.svg in the configured directory.

Current configuration of an SVG icon set:

// Approach 1: either add all icons individually
for (...) {
  matIconRegistry.addSvgIcon('foo', domSanitizer.bypassSecurityTrustResourceUrl('foo.svg'));
}

// Approach 2: or the icon set
matIconRegistry.addSvgIconSet(domSanitizer.bypassSecurityTrustResourceUrl('foo-set.svg'));

Desired configuration:

// Approach 3
matIconRegistry.addSvgIconSetWithPath('/assets/icons/');

Use Case

The primary goal is to reduce the size of the app the user loads. Popular svg sprites are large and most of those items are not used in apps.

Adding icons individually (approach 1) works, but requires to maintain a list of all used icons or having a list of all possible icons (e.g. a string array of icon names) which again adds unnecessary data that has to be downloaded.

There should be a third approach that solves the issue and only actually loads the icons and metadata that are used by an app.

@philmtd philmtd added the feature This issue represents a new feature or feature request rather than a bug or bug fix label Feb 25, 2020
@crisbeto
Copy link
Member

We use the URL to fetch the icon using an HTTP request. I'm not sure how that would work out when passing in a pattern. We also have support for defining an icon as a string so you could use Webpack to import the icons as a pattern (e.g. require.context('./icons', true, /\.svg$/)) and then register the values yourself.

@philmtd
Copy link
Author

philmtd commented Feb 25, 2020

We use the URL to fetch the icon using an HTTP request. I'm not sure how that would work out when passing in a pattern.

The icons would have to reside in a directory (which "defines" the icon set), e.g. "assets/icons". The URL would have to be composed by using the path to the directory and the icon name plus the svg file extension (e.g. ${iconSetPath}/${svgIconName}.svg). It would only require the file names to be identical with the icon name you pass to the svgIcon attribute.

I think to add the functionality a method similar to the following would have to be added in the icon registry (maybe SvgIconConfig would be the wrong type but just for the sake of the example I kept it here...):

// something similar to this:
private _loadNamedSvgIconFromConfigPath(iconName: string, config: SvgIconConfig): Observable<SVGElement> {
    return this._fetchUrl(`${config.url}/${iconName}.svg`)
        .pipe(map(svgText => this._createSvgElementForSingleIcon(svgText, config.options)));
}

We also have support for defining an icon as a string so you could use Webpack to import the icons as a pattern (e.g. require.context('./icons', true, /.svg$/)) and then register the values yourself.

Yes but, as in my described approach 1 developers still would have to take care of maintaining "a list" of the icons they used, right? Otherwise, if I'd just put 1000+ icons in a directory, webpack would include them all and register them regardless of whether I only used 5 of those icons or all of them. This still leads to a user downloading more code than necessary because the code that registers 1000 icons is always executed, even if the corresponding icons are never used.

Having the possibility to just tell the icon registry "In directory X are my icons, please look them up there." would solve the issue. No individual registration required (less code) and only used icons are downloaded.

@philmtd
Copy link
Author

philmtd commented Feb 26, 2020

What do you think of this approach, @crisbeto?

@crisbeto
Copy link
Member

crisbeto commented Mar 8, 2020

I think that it would make more sense to allow for a function to be passed in that allows people to construct the URL themselves, rather than coming up some kind of templating. I'd wait until #18654 gets in before changing anything since it moves some things around.

@mmalerba mmalerba added the needs triage This issue needs to be triaged by the team label May 20, 2020
@crisbeto crisbeto added area: material/icon P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels May 27, 2020
@crisbeto crisbeto self-assigned this Dec 24, 2020
crisbeto added a commit to crisbeto/material2 that referenced this issue Dec 24, 2020
Currently all of the ways to register icons (e.g. via URL or literal) require the developer to
know the list of icons ahead of time or to fetch more icons than the user needs. These
changes aim to address this issue by adding the ability to register an icon resolver
function which will construct the URL at runtime, depending on the requested icon.

Fixes angular#18607.
@crisbeto
Copy link
Member

@philmtd it's been a while since we discussed it, but I finally got back around to this issue. I've submitted #21431 which adds the ability to register a function that resolves the icon URL at runtime. Can you take a look if this is something that would've worked for your use case?

crisbeto added a commit to crisbeto/material2 that referenced this issue Dec 24, 2020
Currently all of the ways to register icons (e.g. via URL or literal) require the developer to
know the list of icons ahead of time or to fetch more icons than the user needs. These
changes aim to address this issue by adding the ability to register an icon resolver
function which will construct the URL at runtime, depending on the requested icon.

Fixes angular#18607.
@philmtd
Copy link
Author

philmtd commented Dec 28, 2020

@crisbeto Thanks, I took a look at your changes and they look good to me.

Getting back to my original example I can now do the following if I want to resolve all icons in the default namespace manually:

matIconRegistry.addSvgIconResolver((name, namespace) => {
  return namespace === "" ? domSanitizer.bypassSecurityTrustResourceUrl(`/assets/icons/${name}.svg`) : null;
});

Can't wait to use it once it's released!

annieyw pushed a commit that referenced this issue Jan 9, 2021
Currently all of the ways to register icons (e.g. via URL or literal) require the developer to
know the list of icons ahead of time or to fetch more icons than the user needs. These
changes aim to address this issue by adding the ability to register an icon resolver
function which will construct the URL at runtime, depending on the requested icon.

Fixes #18607.
wagnermaciel pushed a commit to wagnermaciel/components that referenced this issue Jan 14, 2021
…lar#21431)

Currently all of the ways to register icons (e.g. via URL or literal) require the developer to
know the list of icons ahead of time or to fetch more icons than the user needs. These
changes aim to address this issue by adding the ability to register an icon resolver
function which will construct the URL at runtime, depending on the requested icon.

Fixes angular#18607.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: material/icon feature This issue represents a new feature or feature request rather than a bug or bug fix P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants