-
-
Notifications
You must be signed in to change notification settings - Fork 974
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
Use the fontawesome
package for the HTML dependency on Font Awesome
#2451
Conversation
Did you test R Markdown website too or maybe Flexdashboard ? I think they use the current dependency from |
Those are good candidates for manual testing. Will definitely test those as well. Thanks! |
Some general thoughts after thinking about the topic to maybe discuss live together: I wonder if we have a benefit to reexport the function from rmarkdown directly. I believe it was done to be used in other package like
However we are indeed using it for navbar in website (see #2042 that needs to be dealt with after / with this topic - mentioning so that it is on your radar) - so we need to use the package anyway in rmarkdown One scenario I had in mind could be for us to
However, before fontawesome package, @rich-iannone @yihui some thoughts on that ? |
I took the source files from this flexdashboard example (https://testing-apps.shinyapps.io/flexdashboard-shiny-crandash/) and ran it locally. The FA icons appear in their value boxes; they do use the rmarkdown HTML dependency function directly. |
I'm more in favor of the non-deprecation route. Also I would like to fix #2042. Perhaps, before merging, we should talk about all the related issues at our next meeting (or sooner). |
I agree. |
htmlDependency( | ||
"font-awesome", | ||
"5.1.0", | ||
src = pkg_file("rmd/h/fontawesome"), |
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.
We can probably delete the directory inst/rmd/h/fontawesome
, too, but it depends on whether we want to Import
or Suggest
fontawesome. If we want to make fontawesome a soft dependency, we can keep our (outdated) version of fontawesome. I don't have a strong opinion on this. The scope of the problems that we are trying to resolve seems to be narrow in my eyes (i.e., the icons are not actually widely used in this package), so I'm not sure if it's worth a new hard dependency.
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.
The scope of the problems that we are trying to resolve seems to be narrow in my eyes (i.e., the icons are not actually widely used in this package), so I'm not sure if it's worth a new hard dependency.
That is why at first as was thinking about deprecated to remove this function in the long term as user should use fontawesome 📦 directly and not through rmarkdown.
If we want to make fontawesome a soft dependency
This would mean that:
- if fontawesome is available, then we use its HTML dependency
- if not available we fallback to our internal one, even if outdated
Is that right ?
That could be a in-between solution to not Import the package indeed. In that case, I would probably add a message or a warning (probably using xfun::do_once()
) to state that an outdated fontawesome would be used and the fontawesome would be used. But one could argue that this is like doing a soft deprecation probably 😅
We could check if an icon is available or not in our version, but probably too much complicated to do and maintain for the benefit.
It is just that silently doing the fallback will not prevent user to not understand why an icon is not showing, and also not teach people about the fontawesome package.
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.
I thought a bit more about this and came away with the conclusion that having to install fontawesome
separately might be confusing to a lot of users and it could cause problems when publishing across different environments. Usually for me when there's an 'optional' package there's always a clear error and message to install that optional package (for extra functionality) when using a specific function or argument. But in the case of a soft dependency it could happen that users would have icons appearing or not, or, older versions of icons being shown (not to mention the difficulty of explaining the different versions of the dependency in the documentation).
I agree that an extra dependency is not to be taken lightly but especially given that other packages like flexdashboard
use the dependency function from rmarkdown
directly (and they are more likely to use icons) this makes the case even stronger for fontawesome
to be included in Imports
.
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.
Is that right ?
Yes.
Given all the potential trouble, we should probably just make fontawesome a hard dependency.
Thanks for the discussion on this. I've removed the directory that contained the FA font files ( |
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.
Ok let's do it that way for now, and we can reduce the dependencies in the future if needed.
Thanks @rich-iannone !
@yihui ok for you to merge ? |
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.
Yes, I'll merge it in a minute.
[ci skip]
Merge remote-tracking branch 'rstudio/main' into jg-devel * rstudio/main: Use the `fontawesome` package for the HTML dependency on Font Awesome (rstudio#2451) Ensure that rmarkdown::reneder() always returns invisibly (rstudio#2452) use knitr::is_latex_output() in case the output format name contains Pandoc extensions, e.g., `latex-smart` # Conflicts: # NEWS.md
* jg-devel: Use the `fontawesome` package for the HTML dependency on Font Awesome (rstudio#2451) Ensure that rmarkdown::reneder() always returns invisibly (rstudio#2452) use knitr::is_latex_output() in case the output format name contains Pandoc extensions, e.g., `latex-smart` start the next version CRAN release v2.20 make sure `ioslides_presentation` is properly self-contained when requested (rstudio#2428) More tests needs fixing Newer Pandoc (currently devel) fixes GFM toc previously written in HTML instead of Markdown make sure to avoid creating invalid paths when copying resources (rstudio#2429) Fix rstudio#1508: do not resolve input files that are symlinks (rstudio#2438)
Merge remote-tracking branch 'rstudio/main' into jg-tree-fix * rstudio/main: Use the `fontawesome` package for the HTML dependency on Font Awesome (rstudio#2451) Ensure that rmarkdown::reneder() always returns invisibly (rstudio#2452) use knitr::is_latex_output() in case the output format name contains Pandoc extensions, e.g., `latex-smart` # Conflicts: # NEWS.md
The
fontawesome
package maintains an HTML dependency function that Shiny currently uses to generate FA icons. With this PR, thefontawesome
package is taken as a dependency itself (inImports
) and thefontawesome::fa_html_dependency()
is invoked insidehtml_dependency_font_awesome()
(to keep things simple).I performed several tests to ensure icons from several generations of Font Awesome appear. I also tested with
self-contained: true
,self-contained: false
, andruntime: shiny
and everything passed (FA icon always gets rendered). Here is the R Markdown document that tests HTML output with several generations of FA icons:The rendering of icons has no problems and it matches icons obtained from another method (using
fontawesome::fa()
):Fixes: #2276
Fixes: #1661