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

Insure fontawesome V4 compatibility in navbar #1994

Merged
merged 7 commits into from
May 18, 2021
Merged

Insure fontawesome V4 compatibility in navbar #1994

merged 7 commits into from
May 18, 2021

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Dec 22, 2020

This aims to fix #1991. The issue is that fas is now the default prefix instead of deprecated fa prefix, so we used that in #1967. However, rmarkdown was previously taking care of adding fa prefix to icon in navbar.
And it was working because fontawesome as shims for V4 compatibility (in v4-shims.css). For example, this was working:

navbar:
  right:
  - icon: fa-github
    href: https://github.com/xxx/yyy

because fa was added leading to fa fa-github which is taking care of by the V4 shims compatibility layer.
Our change would lead to add fas in this case, leading to fas fa-github which is wrong as it should be fab fa-github

As we don't really have a way to know if fas or fab should be added without parsing the list of icons, I think it is enough we no more add any prefix in icon in navbar for fontawesome 5.

This means :

  • prefix + name needs to be passed for fontawesome 5 icons, like for new brand icons fab fa-r-project (original issue in Support for Font Awesome 5 glyphs in navbar #1554)
  • fa-<iconname> will always be prefixed with fa (fa fa-<iconname>) not fas. This will insure V4 compatibility. All the more because it seems that in V5, fa can still be used even if the default is fas now.
  • If for some reason some pass fa fa-<iconname> directly, don't duplicate the prefix and keep it that way.

I think this should be good now.

Adding fas by default will break V4 compatibility. New handling will be :
- Use fill prefix + name for V5+
- support V4 fa + name if passed
- Still add deprecated fa prefix if no prefix is passed for V4 compatibility
@jdblischak
Copy link
Contributor

@cderv A small request: would you be able to add a bullet to NEWS.md to document this change? And if possible, it would be ideal if you could add a bullet to the 2.6 release to note that a change regarding Font Awesome was made. I heavily rely on NEWS.md to help keep workflowr up-to-date with the latest changes to rmarkdown. Great work by the way. It's awesome that you're now part of the official rmarkdown development team.

@cderv
Copy link
Collaborator Author

cderv commented Dec 22, 2020

I already added the NEWS for previous change b71f9b0

and yes I'll add one here - usually I do that before merging but I should do earlier to not forget maybe. Thanks !

@cderv
Copy link
Collaborator Author

cderv commented Feb 10, 2021

@jdblischak I may rethink this in the future (and it is part of why this is not merged).
I think there is something we can do (like shiny) to avoid providing a prefix and add the correct one if missing.
tracked in #2042

@jdblischak
Copy link
Contributor

@cderv Thanks for the update. My current workaround is to explicitly add the "fab" prefix for rmarkdown >= 2.6. Will this continue to work even if you add the feature to automatically add "fab"? My reading of your description in #2042 is that "if needed" means my workaround will continue to work.

# Get Font Awesome icon for a brand
#
# Decide between Font Awesome 4 and 5
# https://github.com/rstudio/rmarkdown/issues/1991
get_fa_brand_icon <- function(brand) {
  if (utils::packageVersion("rmarkdown") < "2.6") {
    # Font Awesome 4: rmarkdown adds preceding "fa"
    return(sprintf("fa-%s", brand))
  }

  # Font Awesome 5
  return(sprintf("fab fa-%s", brand))
}

@cderv
Copy link
Collaborator Author

cderv commented Feb 12, 2021

Yes if a prefix is added I believe we should not touch it and leave as-is.

But this also would mean if we follow this road that you may not need this workaround anymore as that it what rmarkdown would do for you.

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

On second thought, I tend to merge this PR for now instead of waiting for the fontawesome package to get rid of its heavy dependencies. BTW, depending on a completely new CRAN package means its binaries won't be available for older versions of R, which could be a trouble for Windows users, so I'd wait for even longer before we use fontawesome in rmarkdown (perhaps after three or five new versions of R have been released).

Anyway, I'll let you decide on this one. Thanks!

@cderv cderv added the next to consider for next release label May 18, 2021
@cderv
Copy link
Collaborator Author

cderv commented May 18, 2021

I'll merge this as this is an expected fix and I do not know when I'll work on adding direct fontawesome package support. Thanks @yihui.

@cderv cderv changed the title Insure fontawesome V4 compatibility in navbar to avoid breaking change Insure fontawesome V4 compatibility in navbar May 18, 2021
@cderv cderv merged commit f798b78 into master May 18, 2021
@cderv cderv deleted the fa-5-icons branch May 18, 2021 14:41
@cderv cderv added this to the v2.9 milestone May 18, 2021
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Jun 18, 2021
* rstudio_origin/master: (90 commits)
  start the next version
  CRAN release v2.9
  fix rstudio#2163: do not normalize web paths in the css argument of html output formats (rstudio#2164)
  Add a css file for future tests
  Restore previous shiny theme when run() exits (rstudio#2160)
  Correct name in workflow file
  Add a workflow to test Pandoc nighly (rstudio#2153)
  bslib 0.2.5.1 is now on CRAN
  use `stop2()` from knitr internal instead of `stop(..., call. = FALSE)` (rstudio#2152)
  74e2f2f changed the error message, and a test relies on the content of this message (it probably shouldn't be so specific)
  tweak wording
  Support 'href' dependencies (rstudio#2151)
  Loop correctly on splitted file by file_scope function (rstudio#2150)
  update snapshot file following change in test name in a8aad75
  amend 84ff469: clarify the meaning of fig_crop = 'auto' in the doc
  Add the position of the last character (rstudio#2146)
  don't put the checklist in the comment since it is required
  Fix for lost encoding in shiny_prerendered_html. (rstudio#2140)
  Insure fontawesome V4 compatibility in navbar (rstudio#1994)
  fix rstudio#2043: replace parse(text) with xfun::parse_only() to avoid hanging the R session when the input is empty
  ...
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Jun 18, 2021
Merge remote-tracking branch 'rstudio_origin/master' into jg-devel

* rstudio_origin/master: (64 commits)
  start the next version
  CRAN release v2.9
  fix rstudio#2163: do not normalize web paths in the css argument of html output formats (rstudio#2164)
  Add a css file for future tests
  Restore previous shiny theme when run() exits (rstudio#2160)
  Correct name in workflow file
  Add a workflow to test Pandoc nighly (rstudio#2153)
  bslib 0.2.5.1 is now on CRAN
  use `stop2()` from knitr internal instead of `stop(..., call. = FALSE)` (rstudio#2152)
  74e2f2f changed the error message, and a test relies on the content of this message (it probably shouldn't be so specific)
  tweak wording
  Support 'href' dependencies (rstudio#2151)
  Loop correctly on splitted file by file_scope function (rstudio#2150)
  update snapshot file following change in test name in a8aad75
  amend 84ff469: clarify the meaning of fig_crop = 'auto' in the doc
  Add the position of the last character (rstudio#2146)
  don't put the checklist in the comment since it is required
  Fix for lost encoding in shiny_prerendered_html. (rstudio#2140)
  Insure fontawesome V4 compatibility in navbar (rstudio#1994)
  fix rstudio#2043: replace parse(text) with xfun::parse_only() to avoid hanging the R session when the input is empty
  ...

# Conflicts:
#	DESCRIPTION
#	R/html_dependencies.R
jonathan-g added a commit to jonathan-g/rmarkdown that referenced this pull request Jun 18, 2021
Merge remote-tracking branch 'rstudio_origin/master' into minimal-tree-fix

* rstudio_origin/master: (223 commits)
  start the next version
  CRAN release v2.9
  fix rstudio#2163: do not normalize web paths in the css argument of html output formats (rstudio#2164)
  Add a css file for future tests
  Restore previous shiny theme when run() exits (rstudio#2160)
  Correct name in workflow file
  Add a workflow to test Pandoc nighly (rstudio#2153)
  bslib 0.2.5.1 is now on CRAN
  use `stop2()` from knitr internal instead of `stop(..., call. = FALSE)` (rstudio#2152)
  74e2f2f changed the error message, and a test relies on the content of this message (it probably shouldn't be so specific)
  tweak wording
  Support 'href' dependencies (rstudio#2151)
  Loop correctly on splitted file by file_scope function (rstudio#2150)
  update snapshot file following change in test name in a8aad75
  amend 84ff469: clarify the meaning of fig_crop = 'auto' in the doc
  Add the position of the last character (rstudio#2146)
  don't put the checklist in the comment since it is required
  Fix for lost encoding in shiny_prerendered_html. (rstudio#2140)
  Insure fontawesome V4 compatibility in navbar (rstudio#1994)
  fix rstudio#2043: replace parse(text) with xfun::parse_only() to avoid hanging the R session when the input is empty
  ...

# Conflicts:
#	DESCRIPTION
#	NEWS.md
#	R/html_dependencies.R
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
next to consider for next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Brand icons like fa-github no longer work with rmarkdown 2.6
3 participants