-
Notifications
You must be signed in to change notification settings - Fork 904
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
Upgrade to FontAwesome 6 #1102
Upgrade to FontAwesome 6 #1102
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Will take a look - @chalin, can you take a look as well? Agree would be good to have the latest version for the next Docsy release. It doesn't look like it's likely to break anything but do FontAwesome themselves document any breaking changes? Don't want any users to suddenly end up with unexpected icons. |
Will take a look as soon as I can. |
27589f5
to
c9f4971
Compare
@chalin conflicts resolved. |
Backward compatibility information from the project: https://fontawesome.com/docs/web/setup/upgrade/whats-changed#backward-compatibility Lots of icons got renamed, but the v5 names will still work. It'd be worth pointing to this table in the release notes, though, to make people aware that they should pick up the new names at some point. |
fb49386
to
3b9dd25
Compare
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. Note that I haven't done an exhaustive search to see if any FA class names might have been missed, but what has been updated here works. Thanks!
See inline comment for suggested improvements for this or a followup PR.
Thanks @samiahmedsiddiqui for the PR. Thanks @polarweasel for the link to the backwards compatibility page. FYI, some prep work which I'd like reviewed and merged before this PR: Comments are welcome over on that PR. |
1ef1f1b
to
ab588ab
Compare
ab588ab
to
ff626fb
Compare
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.
See inline comment regarding an issue with search placeholder text styling.
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.
Very nice step forward, doing the bulk of the upgrade. Will merge now (as agreed during PSC meeting) and follow up with further changes.
I've collected the followup tasks in: |
(Edit: @chalin)
Preview: https://deploy-preview-1102--docsydocs.netlify.app/docs/