-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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(material/icon): SEO friendly ligature icons #24578
Conversation
9e96870
to
90317c3
Compare
90317c3
to
1c554bf
Compare
src/material/icon/icon.ts
Outdated
@@ -140,7 +143,7 @@ const funcIriPattern = /^url\(['"]?#(.*?)['"]?\)$/; | |||
'role': 'img', | |||
'class': 'mat-icon notranslate', | |||
'[attr.data-mat-icon-type]': '_usingFontIcon() ? "font" : "svg"', | |||
'[attr.data-mat-icon-name]': '_svgName || fontIcon', | |||
'[attr.data-mat-icon-name]': '_svgName || fontIcon || icon', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we reuse the existing fontIcon
input for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I went with a different attribute is to avoid as much as possible to change existing behavior. If we reuse fontIcon
, then that means all pre-existing usage of it will now have the new CSS applied to them. And that would break usage of non-ligature icons, such as FontAwesome, as seen in this alternative prototype:
So I guess technically we could still re-use fontIcon
, but if and only if we have a separate way to tell which fontSet is ligature or not. I suppose we could do something in MatIconRegistry
for that, but that seemed over-complicated to me. Is it still something you would like to explore ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing this in JS and introducing a new input, couldn't we have a CSS class that people can specify if they're using ligature icons? The class can be set through MatIconRegistry.setDefaultFontSetClass
so all this PR would have to do would be to add the class and document how to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea to me. I'll update the PR soonish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crisbeto I force pushed the new alternative. The resulting API seems cleaner this way. Let me know if some adjustments are needed.
4b54d56
to
c4abd53
Compare
@andrewseguin I rebased the PR as asked. Could you please finish the review ? |
@crisbeto Would you mind reviewing this? |
bca0ec1
to
f54262f
Compare
The new way to use ligature icons, via the `fontIcon` attribute, allows to hide the font name from search engine results. Otherwise, the font name, which was never intended to be read by any end-users, would appear in the middle of legit sentences in search results, thus making the search result very confusing to read. New recommended usage is: ```diff - <mat-icon>home</mat-icon> + <mat-icon fontIcon="home"></mat-icon> ``` To also enable this for custom font, include the special `mat-ligature-font` class when registering the font alias. So like this: ```ts iconRegistry.registerFontClassAlias('f1', 'font1 mat-ligature-font'); ``` And use like this: ```html <mat-icon fontSet="f1" fontIcon="home"></mat-icon> ``` Fixes angular#23195 Fixes angular#23183 Fixes google/material-design-icons#498
f54262f
to
8a82059
Compare
@@ -149,7 +149,7 @@ export class MatIconRegistry implements OnDestroy { | |||
* specified. The default 'material-icons' value assumes that the material icon font has been | |||
* loaded as described at http://google.github.io/material-design-icons/#icon-font-for-the-web | |||
*/ | |||
private _defaultFontSetClass = ['material-icons']; | |||
private _defaultFontSetClass = ['material-icons', 'mat-ligature-font']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that adding it to the defaults may break existing clients. We should document in icon.md
how people can opt into it instead. A live example would be good as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crisbeto, sorry for the unusually late reply.
I don't think it is a breaking change, because even though the new class is always added, it only has any effect if and only if the fontIcon
attribute is also used. And since that attribute did not have any meaning for the material-icons font previously, there is no reason to have that in existing code. And even then if a user incorrectly duplicated the icon name, like <mat-icon fontIcon="home">home</mat-icon>
, then it would only show the icon over each other. So even in case of past incorrect use, there is not visual difference at all.
Also I don't think that having a new class (which does nothing) on a element is considered breaking, because classes are prefixed with mat-
specifically to avoid class name conflicts.
Below is an extract of the dev-app where you can see that new usage style (by attribute), and the old usage style (by content) are both pixel perfect, without any changes for the old usage:
What exactly did you have in mind when you mentioned breaking change ?
Can we still get this merged for the v14 release ? |
@crisbeto I double-checked this with a TGP and found no failing internal tests. Do you think this can go in as-is? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The new way to use ligature icons, via the `fontIcon` attribute, allows to hide the font name from search engine results. Otherwise, the font name, which was never intended to be read by any end-users, would appear in the middle of legit sentences in search results, thus making the search result very confusing to read. New recommended usage is: ```diff - <mat-icon>home</mat-icon> + <mat-icon fontIcon="home"></mat-icon> ``` To also enable this for custom font, include the special `mat-ligature-font` class when registering the font alias. So like this: ```ts iconRegistry.registerFontClassAlias('f1', 'font1 mat-ligature-font'); ``` And use like this: ```html <mat-icon fontSet="f1" fontIcon="home"></mat-icon> ``` Fixes #23195 Fixes #23183 Fixes google/material-design-icons#498 (cherry picked from commit b37c96d)
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/cdk](https://github.com/angular/components) | dependencies | minor | [`14.0.6` -> `14.1.0`](https://renovatebot.com/diffs/npm/@angular%2fcdk/14.0.6/14.1.0) | | [@angular/material](https://github.com/angular/components) | dependencies | minor | [`14.0.6` -> `14.1.0`](https://renovatebot.com/diffs/npm/@angular%2fmaterial/14.0.6/14.1.0) | --- ### Release Notes <details> <summary>angular/components</summary> ### [`v14.1.0`](https://github.com/angular/components/blob/HEAD/CHANGELOG.md#​1410-neon-plate-2022-07-20) [Compare Source](angular/components@14.0.6...14.1.0) ##### cdk | Commit | Type | Description | | -- | -- | -- | | [176213d70](angular/components@176213d) | feat | **scrolling:** make scroller element configurable for virtual scrolling ([#​24394](angular/components#24394)) | ##### material | Commit | Type | Description | | -- | -- | -- | | [1256c6cf2](angular/components@1256c6c) | feat | **core:** use strong focus indicators in high contrast mode | | [dd1a0feb5](angular/components@dd1a0fe) | feat | **icon:** SEO friendly ligature icons ([#​24578](angular/components#24578)) | | [799cf7cf2](angular/components@799cf7c) | fix | **datepicker:** add i18n strings ([#​25024](angular/components#25024)) | ##### cdk-experimental | Commit | Type | Description | | -- | -- | -- | | [5c2a7e00e](angular/components@5c2a7e0) | fix | **listbox:** clean up some TODOs ([#​25005](angular/components#25005)) | | [0b5963753](angular/components@0b59637) | fix | **listbox:** clean up the listbox API and make it work with forms ([#​24920](angular/components#24920)) | ##### material-experimental | Commit | Type | Description | | -- | -- | -- | | [864f92e0f](angular/components@864f92e) | feat | **mdc-button:** support custom leading/trailing icons ([#​24987](angular/components#24987)) | | [1f6810831](angular/components@1f68108) | feat | **mdc-list:** add support for activated state in harness ([#​24934](angular/components#24934)) | | [c543db57a](angular/components@c543db5) | fix | **mdc-checkbox:** remove extra a11y tree node for the <label/> ([#​24907](angular/components#24907)) | | [7736515f1](angular/components@7736515) | fix | **mdc-form-field:** ensure appearance is valid ([#​24963](angular/components#24963)) | #### Special Thanks Adrien Crivelli, Amy Sorto, Andrew Seguin, Jackie Chu, Kristiyan Kostadinov, Maxi, Miles Malerba, Paul Gschwendtner, Wagner Maciel, Zach Arend and atrawally <!-- CHANGELOG SPLIT MARKER --> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTkuMiIsInVwZGF0ZWRJblZlciI6IjMyLjExOS4yIn0=--> Co-authored-by: cabr2-bot <[email protected]> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1470 Reviewed-by: Epsilon_02 <[email protected]> Co-authored-by: Calciumdibromid Bot <[email protected]> Co-committed-by: Calciumdibromid Bot <[email protected]>
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. |
The new way to use ligature icons, via the
fontIcon
attribute, allows to hidethe font name from search engine results. Otherwise, the font name, which was
never intended to be read by any end-users, would appear in the middle of legit
sentences in search results, thus making the search result very confusing to read.
New recommended usage is:
To also enable this for custom font, include the special
mat-ligature-font
classwhen registering the font alias. So like this:
And use like this:
Fixes #23195
Fixes #23183
Fixes google/material-design-icons#498