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

QA: Fix 16,591 HTML Validation Errors #842

Merged
merged 2 commits into from Apr 28, 2015

Conversation

Projects
None yet
2 participants
Contributor

harding commented Apr 26, 2015

Preview: http://dg2.dtrt.org/

This PR:

  1. Enables HTML Proofer's HtmlCheck mode to validate our HTML output, and
  2. Fixes all the errors it found.

Despite the attention-grabbing title, less than a dozen of those errors actually had any affect on the page rendered in Firefox thanks to the forgiving nature of modern browsers.

For details on why various changes were made, please read the commit notes.

I've checked many of the modified link/image/iframe URLs, as well as previewed all of the altered English pages and some of the non-English pages. The altered Bitcoin Core magnet link works in Transmission GUI for Linux.

@saivann saivann commented on the diff Apr 27, 2015

_templates/community.html
@@ -57,7 +57,7 @@ <h2 id="irc"><img src="/img/icons/ico_help.svg" class="titleicon" alt="Icon">{%
</div>
</div>
-<h2 id="nonprofit"><a name="{% translate non-profit anchor.community %}">{% translate nonprofit %}</a></h2>
+<h2 id="nonprofit-organizations"><a name="{% translate non-profit anchor.community %}">{% translate nonprofit %}</a></h2>
@saivann

saivann Apr 27, 2015

Contributor

@harding Why do we need to change this one? I think we're linking to this anchor from other pages, so changing it would break some links.

@harding

harding Apr 27, 2015

Contributor

You're correct that we're linking to the anchor "nonprofit", but that's because we're linking to the translated <a name anchor next to it.

I mention this in the commit notes, that the problem is that the anchor is used twice on the page (the id=nonprofit was added just a couple weeks ago here: https://github.com/bitcoin/bitcoin.org/pull/813/files ).

Also, note that HTML Proofer checks all of our internal links to anchors, so if I had broken an anchor, it would've told us. :-)

@saivann

saivann Apr 27, 2015

Contributor

I was indeed wondering why this didn't raise any error :) That all sounds good to me! I should have looked more carefully.

@saivann saivann commented on an outdated diff Apr 27, 2015

_translations/bg.yml
@@ -563,7 +563,7 @@ bg:
donation: "Направете дарение"
donationtxt: "Най-лесният начин да помогнете е като <a href=\"https://bitcoinfoundation.org/about/donate/\">дарите</a> малко биткойни на Фондация Биткойн. Също така може директно да финансирате проекти, свързани с Биткойн, които смятате, че ще са от полза в бъдещето."
nonprofit: "Неправителствени организации"
- nonprofittxt: "Bitcoin фондация< /a> и много други организации с нестопанска цел< /a> са посветени на защитата и насърчаването Bitcoin. Можете да се помогне на тези групи от присъединяване към тях и участие в проектите им, дискусии и Събития."
+ nonprofittxt: "Bitcoin фондация и много други организации с нестопанска цел са посветени на защитата и насърчаването Bitcoin. Можете да се помогне на тези групи от присъединяване към тях и участие в проектите им, дискусии и Събития."
@saivann

saivann Apr 27, 2015

Contributor

@harding This one appears to have two links missing (see the corresponding English text).

@saivann saivann and 1 other commented on an outdated diff Apr 27, 2015

@@ -8,7 +8,7 @@
title: Bitcoin
---
<p class="mainsummary">{% translate listintro %}</p>
-<div class="mainvideo"><iframe src="//www.youtube.com/embed/Gc2en3nHxA4?rel=0&showinfo=0&wmode=opaque{% if page.lang != 'en' %}&cc_load_policy=1&hl={{ page.lang }}&cc_lang_pref={{ page.lang }}{% endif %}" frameborder="0" allowfullscreen></iframe></div>
+<div class="mainvideo"><iframe src="//www.youtube.com/embed/Gc2en3nHxA4?rel=0&amp;showinfo=0&amp;wmode=opaque{% if page.lang != 'en' %}&cc_load_policy=1&hl={{ page.lang }}&cc_lang_pref={{ page.lang }}{% endif %}" frameborder="0" allowfullscreen></iframe></div>
@saivann

saivann Apr 27, 2015

Contributor

@harding I think there's a few "&" that haven't been replaced here?

@harding

harding Apr 27, 2015

Contributor

I'll fix that in case we reuse that code elsewhere, but they're in a conditional that's never run (the page.lang is always en).

@saivann

saivann Apr 27, 2015

Contributor

Indeed, we are using the same code in the index.html layout.

Contributor

saivann commented Apr 27, 2015

@harding This is fantastic, thank you so much! Will you take care to make the same modifications upstream on Transifex for each translations too?

harding added some commits Apr 26, 2015

QA: Check HTML Correctness & Fix Existing Errors
- _contrib/bco-htmlproof: check HTML for correctness; fail on any errors

- _contrib/bco-htmlproof: accept path for individual page to help debug
  page problems

- (Many files) Convert `&` in numerous elements to `&amp;`

- _templates/download.html: use Liquid filter to automatically escape
  `&` in magnet links.  Also premptively tell HTML not to check the
  magnet link when checking external links (this check is not currently
  enabled)

- _releases/*: Escape `<parameter>` used in multiple Bitcoin Core
  release notes

- _templates/choose-you-wallet.html: change mSigna URL from
  .../coinvault&referer=bitcoin.org to .../coinvault?referer=bitcoin.org

- _templates/community.html: fix duplicate anchors by renaming one
  anchor

- _templates/events.html: move Javascript to separate file because it
  contains forbidden HTML close tags within the `<script></script>`
  tags.

- (Many files, mostly in _translations/) Fix many broken open tags or
  missing close tags.

- _translatios/zh_TW.yml & ko.yml: fix a total of three invalid
  characters (control characters)
QA: Fix Regressions From HTML Proofer Commit
Suggested by Saïvann (thanks!)
Contributor

harding commented Apr 27, 2015

Rebased on current master to make sure everything still checked out with the 0.10.1 release (it did) and added commit 593a56d to fix the two regressions @saivann identified. (Thanks!)

@saivann I wasn't planning to mess with Transifex. I find the Bitcoin.org/Transifex interface frustrating and I haven't yet put any time into working on it.

If you think that's a blocker for merging this, let me know.

Otherwise, in the absence of critical feedback, I'll merge this around 13:00 UTC Tuesday.

@harding harding merged commit 593a56d into bitcoin-dot-org:master Apr 28, 2015

1 check passed

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

harding added a commit that referenced this pull request Apr 28, 2015

Merge #837, #838, and #842
* #837: Add A Blog To Bitcoin.org
* #838: Full Node Guide: Add Windows 8.x Instructions
* #842: QA: Fix 16,591 HTML Validation Errors
Contributor

saivann commented Apr 28, 2015

@harding I just updated translations by hand on Transifex, and will take care of updating en.yml as well.

Contributor

harding commented Apr 28, 2015

@saivann wow, thanks!

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