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

Add brand prefixes for fontawesome icons #46

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

apreshill
Copy link
Contributor

Noticed some of your icons in the upper navbar were broken, this one bit me too!

Noticed some of your icons in the upper navbar was broken, this one bit me too!
@codecov-io
Copy link

Codecov Report

Merging #46 (80c3391) into master (f2edaf0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #46   +/-   ##
=======================================
  Coverage   94.68%   94.68%           
=======================================
  Files          17       17           
  Lines         734      734           
=======================================
  Hits          695      695           
  Misses         39       39           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2edaf0...80c3391. Read the comment docs.

@gadenbuie gadenbuie self-requested a review February 9, 2021 14:31
Copy link
Owner

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Thanks! I've seen this in more than a few places and have been meaning to dig into it!

@gadenbuie gadenbuie merged commit 300908a into gadenbuie:master Feb 9, 2021
@cderv
Copy link

cderv commented Feb 9, 2021

No need to dig it more, this is a known issue rstudio/rmarkdown#1991 and there is already a PR that I think solves it.

Sorry for the trouble 😄

@gadenbuie
Copy link
Owner

Thanks @cderv (for telling me after I went and updated 6 other repos 😜)! Seriously though, am I reading that issue right that you'll make FA4 work but that using the FA5 prefixes is probably the safest option in the future?

@cderv
Copy link

cderv commented Feb 9, 2021

Seriously though, am I reading that issue right that you'll make FA4 work but that using the FA5 prefixes is probably the safest option in the future?

The story is that

So FA4 will work without the prefix changes for what is included in the shims file for compatibility (https://github.com/rstudio/rmarkdown/blob/master/inst/rmd/h/fontawesome/css/v4-shims.css following https://fontawesome.com/how-to-use/on-the-web/setup/upgrading-from-version-4#manually).
So rmarkdown will do as before and add fa prefix for you. This will work for everything that works with the compatibility layer, but it is best to provide the correct prefix to select icons now.

Does it seem not right to you ?

We could try being more clever and try to decide the right prefix, but it seemed a bit too much. If you want to dicuss it, I'll be happy to.

@cderv
Copy link

cderv commented Feb 10, 2021

In fact, I looked into how shiny handles it and they maintain a list of fontawesome brand icon. When one is detected the fab prefix is used.

I think this can follow up this path to make easily updated and so that just the name of the icon can be passed. No prefix needed then.

Thanks for challenging this !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants