Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Start using Font Awesome icons #1007

Merged
merged 3 commits into from Aug 25, 2015

Conversation

Projects
None yet
4 participants
Contributor

harding commented Aug 10, 2015

Preview: http://dg0.dtrt.org/en/developer-documentation

This pull request adds the Font Awesome iconic font to the site (available on all pages) and uses it for the Developer Documentation main page as a demo.

Although I think our current icons are fine, I need additional icons for some new pages I hope to add and I would prefer to reuse existing work rather than create my own. Font Awesome has the following
benefits:

  1. It's popular, which hopefully means that it will be maintained and improved by other people, allowing us to spend more time focusing on Bitcoin and less time on making and maintaining icons.
  2. It's easy to customize using only CSS. In the examples below, the images are all grown beyond their default sizes, the images that appear in links inherit our link color, and one of the images is rotated 270 degrees---all done via CSS classes. There are even more possibilities.
  3. It has a lot of icons to choose from, and they're packed into a tiny amount of space (106K for the SVG font file after gzipping).

Licensing

Font Awesome freely licensed under the SIL Open Font License, an OSI approved license and which the FSF considers a free license but not GPL-compatible. IANAL, but I don't see any conflicts with our MIT-licensed content nor the small amount of non-freely-licensed content we have.

Note: the Font Awesome license page says it is "GPL friendly". They have an issue that goes into detail about why they're not fully GPL compatible.

Compatibility

  • Broken: IE7
  • Broken: Firefox with NoScript & default settings---will not load @font-faces from anywhere, even the same server (details here that make it sound like a reasonable security policy). I worked around this by hiding Font Awesome icons when JS is disabled.
  • Tested works: IE8 (tested with NetRender)
  • Tested works: recentish Firefox and Chromium with default settings, and also in Chromium with JS disabled through the regular settings menu
  • Tested works: Android 2.3.5 browser (which doesn't support SVG, but does load Font Awesome's alternative font files)

Font source

In this PR are the files from this ZIP archive unchanged, plus a COPYING file I added with the text from this License page.

Side-by-side example

On the left side is the page as currently displayed on Bitcoin.org; on the right side is the preview page with all of the images from Font Awesome:

screenshot-dg0 dtrt org 2015-08-10 13-20-45

harding added some commits Aug 10, 2015

Contributor

saivann commented Aug 17, 2015

I haven't tested and reviewed the PR yet, but I agree this can be a good idea. Perhaps this can be loaded only on the pages using it to avoid unnecessary blocking CSS and bandwidth consumption on mobiles? I think this adds a few hundred kb at worst?

Some of our icons looked more appropriate in the context I think (like the miners icon), but if fontawesome saves us time, that's really not a huge issue IMO.

Contributor

harding commented Aug 17, 2015

@saivann I justed tested on the preview site linked above, and the CSS is always loaded but the icons are only loaded when used. (See screenshots below.)

According to Firefox, the CSS is 26 KB. Since that should be cached on first load, I think it's low enough to keep as a default addition to every page. However, if you want, I'm entirely happy to make adding it to a page's HTML a setting in the YAML header. Let me know which you prefer.

Load of page (no cache) that uses Font Awesome icons:

2015-08-17-181245_1440x900_scrot

Load of page (no cache) that doesn't use the icons:

2015-08-17-181350_1440x900_scrot

Contributor

saivann commented Aug 17, 2015

@harding That's actually more optimized than I expected. Agreed, thanks!

Contributor

harding commented Aug 17, 2015

@saivann Cool, thanks! Let me know when you've had a chance to test and I'll schedule the merge (but there's no rush). @crwatkins has already tested on iOS and didn't see any problems.

Contributor

ChainQuery commented Aug 20, 2015

Contributor

luke-jr commented Aug 20, 2015

SIL OFL is not a free license, despite its wide acceptance. :(

Contributor

harding commented Aug 20, 2015

@ChainQuery I think that's a reasonable icon. I chose the puzzle icon because proof-of-work is sometimes described as analogous to a "scratch-off puzzle" (particularly by Andrew Miller, IIRC). I don't have any strong opinion here, so if someone else wants to suggest one or the other (or a third option), just say so.

@luke-jr yes, its GPL incompatibility is mentioned in the Licensing section of the OP.

@saivann saivann and 1 other commented on an outdated diff Aug 21, 2015

_includes/layout/base/html-head.html
@@ -8,6 +8,11 @@
<meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
<title>{% capture title %}{% translate title %}{% endcapture %}{% if title != '' %}{{ title }}{% else %}{{ page.title }}{% endif %}</title>
{% capture metadescription %}{% translate metadescription %}{% endcapture %}{% if metadescription != '' %}<meta name="description" content="{{ metadescription }}">{% endif %}
+
+{% comment %}<!-- load Font Awesome but hide it if JS is disabled -->{% endcomment %}
+<link rel="stylesheet" href="/css/font-awesome-4.4.0/css/font-awesome.min.css">
+<noscript><style>.fa { display: none !important; }</style></noscript>
@saivann

saivann Aug 21, 2015

Contributor

@harding I think we could actually avoid this extra line (Edit: and the comment describing it), as it also has the unintended consequence of hiding the icons on browsers with javascript disabled (they display correctly on FF3.5,40 with JS disabled). When we let icons show while NoScript is enabled, the result doesn't look particularly bad to me compared with the other option of just showing no icons (screenshot attached). Also, I have heard that Google doesn't like noscript tags.

capture du 2015-08-21 15 14 54

@harding

harding Aug 21, 2015

Contributor

@saivann Agreed. I'm a pretty serious NoScript user and I've become used to seeing those broken unicode characters on pages that use Font Awesome or something similar, so I imagine other users won't be upset to see them on Bitcoin.org.

I'll remove the lines.

@saivann saivann commented on an outdated diff Aug 21, 2015

en/developer-documentation.md
@@ -39,62 +39,62 @@ breadcrumbs:
</div>
<div>
<div>
- <h2 id="contracts"><img src="/img/icons/ico_contract.svg" class="titleicon" alt="Icon">Contracts</h2>
+ <h2 id="contracts"><span class="fa fa-sitemap fa-lg fa-rotate-270"></span> Contracts</h2>
<p><a href="/en/developer-guide#contracts">Contracts Guide</a></p>
<div class="resourcesext">
@saivann

saivann Aug 21, 2015

Contributor

@harding class="resourcesext" doesn't serve anything now and could be removed everywhere from the page.

Contributor

saivann commented Aug 21, 2015

@harding I noticed that the main icons now have a smaller font-size than the rest of the content. If we want to at least make them equally proemient, it's possible to set the .docreference a font-size attribute to 115% and drop the .docreference span+span rule.

Otherwise I have verified that the added files are identical to those in the fontawesome repository. The changes work fine here (IE8,10, FF3.5,39, OP12, Android 2.3.6).

Except for the few comments above, LGTM up to 819b356. Thanks!

Contributor

saivann commented Aug 21, 2015

@ChainQuery Thanks for the alternative suggestion! As far as I'm concerned, I kind of like both icons equally, I guess miners aren't so easily depicted by common icons...

Contributor

harding commented Aug 22, 2015

Commit 073e3f1 makes the changes suggested by @saivann. I left the mining icon the same; we can always change it later.

In the absence of critical feedback, this PR will be merged Tuesday. Thank you everyone who helped review!

Contributor

saivann commented Aug 22, 2015

Commit 073e3f1 LGTM, Thanks!

@harding harding merged commit 073e3f1 into bitcoin-dot-org:master Aug 25, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

harding added a commit that referenced this pull request Aug 25, 2015

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