gadenbuie / xaringanthemer

😎 Give your xaringan slides some style
https://pkg.garrickadenbuie.com/xaringanthemer/
Other
445 stars 28 forks source link

Add brand prefixes for fontawesome icons #46

Closed apreshill closed 3 years ago

apreshill commented 3 years ago

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

codecov-io commented 3 years ago

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.

cderv commented 3 years ago

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

Sorry for the trouble πŸ˜„

gadenbuie commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 !