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

Upgrade FontAwesome to 5.3.1 #2186

Merged
merged 5 commits into from Sep 18, 2018

Conversation

Projects
None yet
3 participants
@alandipert
Contributor

alandipert commented Sep 17, 2018

  • This upgrades FontAwesome to a new major (by itself, breaking) version, but
    is backwards compatible because we include the v4-shims CSS that maps
    old names to new.
  • icon tags are now browsable
  • This is a step toward full V5 adoption that doesn't require us to
    come up with a plan for deprecating V4 icon names.

(Related: #2156 and #1966)

Confirmed that the FontAwesome CSS and webfont licenses didn't change between 4 and 5, added CC license for .svgs we now also include.

@alandipert alandipert requested a review from jcheng5 Sep 17, 2018

htmlDependencies(iconTag) <- htmlDependency(
"font-awesome", "4.7.0", c(href="shared/font-awesome"),
stylesheet = "css/font-awesome.min.css"
if (lib %in% c("font-awesome", "fontawesome")) {

This comment has been minimized.

@alandipert

alandipert Sep 17, 2018

Contributor

Hm I'm not sure about this. Perhaps we should do this:

  • If it's font-awesome, load the shim
  • If it's fontawesome, don't load the shim

Then in subsequent versions of Shiny that ship V5 w/out the shim, specifying font-awesome is an error and fontawesome is the new deault lib

This comment has been minimized.

@jcheng5

jcheng5 Sep 17, 2018

Member

What was the reason for adding fontawesome as a valid option in the first place?

This comment has been minimized.

@alandipert

alandipert Sep 18, 2018

Contributor

A half-baked scheme for supporting 4 and 5 in a subsequent release, but there's no reason to do it now. Removed and simplified.

htmlDependency(
"fontawesome-v4-shims", "5.3.1", c(href="shared/fontawesome"),
stylesheet = "css/v4-shims.min.css"
)

This comment has been minimized.

@jcheng5

jcheng5 Sep 17, 2018

Member

I think it should be a single htmlDependency named font-awesome, with both stylesheets included. This isn't aesthetic, it matters for dependency resolution that happens when the page is rendered; if the dependency name is different, the page could load both 4.7.0 and 5.3.1, and whichever one is loaded last would win (to the degree that there are collisions, that is).

This comment has been minimized.

@alandipert

alandipert Sep 18, 2018

Contributor

Makes sense. I did it 👍

@eawachtel

This comment has been minimized.

eawachtel commented Sep 18, 2018

I'm really confused on what the current status of fontawesome in shiny is? I'm using shiny v 1.0.5 and have download the fontawesome 4.7 and placed in www/shared. I can not get most icons to work. I see the svg files for the icons I want, but they do not work. Any help appreciated

@jcheng5

This comment has been minimized.

Member

jcheng5 commented Sep 18, 2018

@eawachtel Please file a separate issue if you're having trouble with icons in 4.7.0. It'd be great if you could include a minimal reproducible example. Thanks!

@eawachtel

This comment has been minimized.

eawachtel commented Sep 18, 2018

Upgrade FontAwesome to 5.3.1
- Upgrades FontAwesome to a new major (breaking) version, but
  is backwards compatible because we include the v4-shims CSS that maps
  old names to new.
- This is a step toward full V5 adoption that doesn't require us to
  come up with a plan for deprecating V4 icon names.
- Details: https://fontawesome.com/how-to-use/on-the-web/setup/upgrading-from-version-4
- Related to #2156 and #1966

@alandipert alandipert force-pushed the alan/upgrade-font-awesome branch from c6195d6 to 2396464 Sep 18, 2018

@alandipert

This comment has been minimized.

Contributor

alandipert commented Sep 18, 2018

@eawachtel I think gas-pump doesn't work because it was added in FontAwesome 5, and the currently-released version of Shiny only has FontAwesome 4.7. Once this PR is merged and we've released Shiny 1.2, gas-pump will work.

@jcheng5

This comment has been minimized.

Member

jcheng5 commented Sep 18, 2018

@alandipert Are we missing "CC BY 4.0 License" in the LICENSE file? It looks like it applies to the SVG data, which we appear to be bundling.

alandipert added some commits Sep 18, 2018

Improvements to icon
- Clarify in docs that fontawesome V5 icons accessible with V4-style names
- Make icons browseable: icon('address-book') will now open the Viewer
  pane of RStudio IDE so that icons can be experimented with more easily.

@jcheng5 jcheng5 added this to the 1.2 milestone Sep 18, 2018

@jcheng5 jcheng5 merged commit 3cea5fb into master Sep 18, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jcheng5 jcheng5 deleted the alan/upgrade-font-awesome branch Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment