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

docs(material/icon): update the FontAwesome example link #24954

Merged
merged 3 commits into from
Jun 1, 2022

Conversation

victorwvieira
Copy link
Contributor

@victorwvieira victorwvieira commented May 22, 2022

docs(material/icon): update the FontAwesome link.

Update the FontAwesome link example.
The FontAwesome link was broken.

Fixes #24954

Fixed the FontAwesome link.
@victorwvieira victorwvieira changed the title Update icon.md Update icon.md - Fixed the FontAwesome link May 22, 2022
@amysorto
Copy link
Contributor

Thanks for catching this! Can you update your commit message to follow these Commit Message Guidelines (right now that is causing the lint CI test to fail).

@amysorto amysorto self-requested a review May 27, 2022 16:58
@amysorto amysorto added the target: patch This PR is targeted for the next patch release label May 27, 2022
@victorwvieira
Copy link
Contributor Author

Sorry, my fault. I changed the commit accordingly to the guideline.

@victorwvieira victorwvieira changed the title Update icon.md - Fixed the FontAwesome link docs(material/icon): update the FontAwesome link May 28, 2022
@@ -27,7 +27,7 @@ use the desired font, or to an alias previously registered with

Fonts can also display icons by defining a CSS class for each icon glyph, which typically uses a
`:before` selector to cause the icon to appear.
[FontAwesome](https://fortawesome.github.io/Font-Awesome/examples/) uses this approach to display
[FontAwesome](https://github.com/FortAwesome/angular-fontawesome) uses this approach to display
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this is the correct link since it points to FontAwesome's own Angular component. I think that the old link was showing some examples of the actual icons. Maybe we should link to this instead? https://fontawesome.com/icons

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your point @crisbeto, to be honest I don't know what the broken link was showing, if was just icons examples or implementations examples.
In my situation I was looking for examples of how to start use the FontAwesome and not just the icon itself, than the github angular-fontawesome helped me in the both situation, how to do de integration and also where I can check the icons. This is the reason that I suggested to use this link and not just the icons.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely makes sense not to link to a 404, but the whole point of the doc is to show how to use a font icon set with mat-icon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed should be the example link, you are right @crisbeto.
I will change and do the commit again.

In this case, and seeing that the Fontawesome is also an official Angular component for icons, is it possible to create a session in the documentation to explain how to install and use the Font Awesome in the project?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we should mention the Font Awesome component, because we aren't the ones maintaining it.

docs(material/icon): update the FontAwesome example link.

Update the FontAwesome link to a link with examples.


Fixes angular#24954
@victorwvieira victorwvieira changed the title docs(material/icon): update the FontAwesome link docs(material/icon): update the FontAwesome example link Jun 1, 2022
Update the Font Awesome link to a link with examples.

Fixes angular#24954
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crisbeto crisbeto added docs This issue is related to documentation merge safe merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed target: rc This PR is targeted for the next release-candidate action: merge The PR is ready for merge by the caretaker and removed target: patch This PR is targeted for the next patch release labels Jun 1, 2022
@crisbeto crisbeto merged commit d0f803c into angular:main Jun 1, 2022
crisbeto pushed a commit that referenced this pull request Jun 1, 2022
* Update icon.md

Fixed the FontAwesome link.

* Update icon.md

docs(material/icon): update the FontAwesome example link.

Update the FontAwesome link to a link with examples.

Fixes #24954

* docs(material/icon): update the FontAwesome link

Update the Font Awesome link to a link with examples.

Fixes #24954

(cherry picked from commit d0f803c)
@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 Jul 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker docs This issue is related to documentation merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants