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

Modify icon() function to call fontawesome::fa_i() for equivalent functionality #3302

Merged
merged 40 commits into from
May 12, 2021

Conversation

rich-iannone
Copy link
Member

This PR migrates the dependency files for Font Awesome icon support (font files, CSS, JS) over to the fontawesome package and modifies the shiny::icon() function so that it calls fontawesome::fa_i() to generate the <i> tag object. Other scripts that create supporting Font Awesome objects have been moved over to the fontawesome package and are removed here.

The fontawesome package has been placed in Imports and Remotes (for now, due to be in CRAN shortly). Manual testing has been done using this branch and the release candidate of fontawesome. The demo app 138-icon-fontawesome works with these builds.

@rich-iannone rich-iannone marked this pull request as ready for review February 19, 2021 18:29
@rich-iannone rich-iannone requested a review from wch February 19, 2021 18:29
DESCRIPTION Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
R/bootstrap.R Outdated Show resolved Hide resolved
R/bootstrap.R Outdated Show resolved Hide resolved
man/icon.Rd Show resolved Hide resolved
@rich-iannone rich-iannone requested a review from wch May 6, 2021 15:11
@cpsievert
Copy link
Collaborator

@rich-iannone the unit tests are failing because some of the tabset panel logic relies on this behavior:

> cat(as.character(icon(NULL)))
<i class="" role="presentation" aria-label=" icon"></i>

We should probably do an early return for that case...

@rich-iannone
Copy link
Member Author

Thanks @cpsievert , didn't know about that one and was puzzled by the CI results. I've pushed a change based on your recommendation.

DESCRIPTION Outdated Show resolved Hide resolved
R/bootstrap.R Outdated Show resolved Hide resolved
@wch
Copy link
Collaborator

wch commented May 11, 2021

Would it be possible to get the tabPanel to take a fontawesome::fa() object as the icon?

@cpsievert
Copy link
Collaborator

Would it be possible to get the tabPanel to take a fontawesome::fa() object as the icon?

Yea, but I'm not sure I want to allow icon to be arbitrary HTML(), so I've sent @rich-iannone a PR to have fa() return a shiny.tag rather than an HTML() string https://github.com/rstudio/fontawesome/pull/59/files

@cpsievert cpsievert merged commit d65ad5e into master May 12, 2021
@cpsievert cpsievert deleted the icon-function branch May 12, 2021 19:26
@github-actions github-actions bot restored the icon-function branch May 12, 2021 19:30
schloerke added a commit that referenced this pull request May 17, 2021
* master: (48 commits)
  Modify `icon()` function to call `fontawesome::fa_i()` for equivalent functionality (#3302)
  Follow up to #3372: fix oversight in refactor (#3387)
  Revert "Do not double pull within rituals"
  Do not double pull within rituals
  Remove deprecated code and parameters (#3137)
  Prettify TS
  Rebuild JS files
  Add important flag
  Update comments
  Add sendImageSize2
  Use removeSheet()
  Make more CSS properties !important
  Add note about synchronous behavior in IE
  Rebuild JS files
  Simplify IE CSS handling
  New strategy for sending information when CSS loads
  Make sure dev version of rlang is available (#3382)
  Reduce complexity and 'black-boxed' nature of tab panel logic (#3372)
  Install dev version of rlang (#3379)
  Comment about the hoisting
  ...
@cpsievert cpsievert deleted the icon-function branch October 29, 2021 14:41
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.

3 participants