Use tidy-html5 to validate the term-table page #15577

octopusinvitro · 2016-09-26T16:39:28Z

What does this do?

HTML-validates the term-table page and in doing so addresses this comments

Why was this needed?

HTML was broken

Relevant Issue(s)

everypolitician/everypolitician#505

Implementation notes

None

Screenshots

None

Notes to Reviewer

Errors corrected:

An "img" element must have an "alt" attribute, except under certain
conditions. For details, consult guidance on providing text alternatives
for images. (All occurrences)

Notes to Merger

This branch has been rebased with master. An HTML helper is being introduced as part of #15584, a helper to which this PR would be adding new methods, so that PR should probably be merged first so that adding a new helper is not part of this PR, but just modifying it.

tmtmtmtm

I'm still also quite hesitant of adding such a large fixture for this change. If you only need somewhere that has both a name with double quotes, and also both normal images and placeholders, the popolo for The Bahamas is one sixth of the size.

tmtmtmtm · 2016-09-26T20:25:22Z

t/web/term_table/phillipines.rb

+require 'test_helper'
+require_relative '../../../app'
+
+describe 'Phillipines' do


Typo: "Philippines"

tmtmtmtm · 2016-09-26T20:35:19Z

app.rb

@@ -12,6 +12,7 @@
 require 'everypolitician/popolo'

 require_relative './lib/popolo_helper'
+require_relative './helpers/html'


Shouldn't we be consistent where the helpers live? If we're making a new 'helpers' directory for them, then we should probably move the popolo_helper there too.

I thought about that and I think it may be a good idea. I've seen that in other frameworks as Rails or Middleman and I think it helps to keep concerns separated. But I am happy to put this file under lib if we prefer it so.

tmtmtmtm · 2016-09-26T20:36:37Z

helpers/html.rb

+    end
+  end
+
+  helpers HTMLEscapeHelper


With the PopoloHelper we set it up in app.rb itself. I'm not sure whether it's generally preferred to do it that way or this, but again I think we should be consistent.

See previous comment. I am happy to move it to app.rb though.

tmtmtmtm · 2016-09-26T20:39:47Z

t/web/term_table/phillipines.rb

+
+    it 'doesnt break for names with double quotes' do
+      img = tiangco.css('img.person-card__image')
+      img.attr('alt').text.must_equal 'Member headshot for Tiangco, Tobias "Toby" M.'


There are actually two different images with this text, and only one of them is getting checked here. Both of them actually get returned from this css selector, though, so it should be quite easy to check both.

tmtmtmtm · 2016-09-26T20:41:46Z

helpers/html.rb

+
+module Sinatra
+  module HTMLEscapeHelper
+    def h(text)


Is h a general standard for naming functions like this? I'm generally a fan of brevity, but I'm a little hesitant here, unless this is a universally recognised approach.

Ah, this was copied from the official documentation, I can certainly rename it

octopusinvitro · 2016-09-27T13:43:34Z

@tmtmtmtm Feedback addressed in fixup commits

I tried to call the HTML helper from the app but it couldn't make it work. So I just moved the helper from the helper folder to the lib folder. I also renamed the h method to escape_html.
I downloaded and used the popolo file from the Bahamas and removed the one for the Philippines.
I removed the test file for the Philippines and added a new one for the Bahamas. The switch to the Bahamas file allowed me to discover another bug, when one of the urls in the sources of the data contains spaces. I fixed that by adding another method to the HTMLEscapeHelper called escape_uri which uses URI.escape.

tmtmtmtm · 2016-09-28T08:21:26Z

views/term_table.erb

@@ -198,7 +198,7 @@
      <div class="page-section page-section--grey source-credits">
        <div class="container">
          <% if @page.data_sources %>
-            <p>Main Source<% if @page.data_sources.size > 1 %>s<% end %>: <%= @page.data_sources.map { |url| %Q(<a href="#{url}">#{url}</a>) }.join(", ") %></p>
+            <p>Main Source<% if @page.data_sources.size > 1 %>s<% end %>: <%= @page.data_sources.map { |url| %Q(<a href="#{escape_uri(url)}">#{url}</a>) }.join(", ") %></p>


This approach concerns me. If the problem is that EP is giving us invalid URLs, then we should fix that upstream, so that everyone consuming the data doesn't need to do things like this.

Whichever approach we take, however, this feels like something that deserves a PR of its own, properly explaining the problem and solution etc., rather than cramming more into this PR.

tmtmtmtm · 2016-09-28T08:22:48Z

@octopusinvitro when you say "it couldn't make it work", what was the problem?

The fixture is needed to HTML-validate the term-table page, in particular, the contents of the alt attribute in images, for which a person with a normal image, a person with a placeholder image and a person with double quotes in their name are needed.

Errors corrected: - An "img" element must have an "alt" attribute, except under certain conditions. For details, consult guidance on providing text alternatives for images. (All occurrences)

octopusinvitro · 2016-10-11T15:38:56Z

@tmtmtmtm This branch has been rebased with master. An HTML helper is being introduced as part of #15584, a helper to which this PR would be adding new methods, so that PR should probably be merged first so that adding a new helper is not part of this PR, but just modifying it.

octopusinvitro · 2016-10-14T16:27:23Z

Closed as it was moved to #15587

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15577 September 26, 2016 16:39 Inactive

octopusinvitro added 3 - WIP 4 - Done (Ready for Review) and removed 3 - WIP labels Sep 26, 2016

octopusinvitro assigned tmtmtmtm Sep 26, 2016

octopusinvitro mentioned this pull request Sep 26, 2016

Use tidy-html5 to validate the term-table page #15568

Closed

octopusinvitro force-pushed the fix-closing-tag-3 branch from 91c5e90 to 119ada7 Compare September 26, 2016 16:51

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15577 September 26, 2016 16:51 Inactive

octopusinvitro force-pushed the fix-closing-tag-3 branch from 119ada7 to 5662448 Compare September 26, 2016 16:59

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15577 September 26, 2016 16:59 Inactive

octopusinvitro changed the title ~~Fix closing tag 3~~ Use tidy-html5 to validate the term-table page Sep 26, 2016

tmtmtmtm suggested changes Sep 26, 2016

View reviewed changes

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15577 September 27, 2016 13:34 Inactive

tmtmtmtm suggested changes Sep 28, 2016

View reviewed changes

tmtmtmtm assigned octopusinvitro and unassigned tmtmtmtm Sep 28, 2016

octopusinvitro mentioned this pull request Sep 29, 2016

The URLs are been unencoded before being passed to the template #15583

Closed

octopusinvitro mentioned this pull request Oct 11, 2016

Add html-validation to detect malformed markup #15433

Closed

octopusinvitro added 3 commits October 11, 2016 16:25

Add an HTML escape helper

d5a3c39

Use tidy-html5 to validate the term-table page

fee6b58

Errors corrected: - An "img" element must have an "alt" attribute, except under certain conditions. For details, consult guidance on providing text alternatives for images. (All occurrences)

octopusinvitro force-pushed the fix-closing-tag-3 branch from 3df7f00 to fee6b58 Compare October 11, 2016 15:33

tmtmtmtm deployed to everypolitician-viewe-pr-15577 October 11, 2016 15:33 View deployment

octopusinvitro closed this Oct 14, 2016

tmtmtmtm mentioned this pull request Oct 21, 2016

Use tidy-html5 to validate the term-table page #15587

Open

octopusinvitro deleted the fix-closing-tag-3 branch November 18, 2016 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use tidy-html5 to validate the term-table page #15577

Use tidy-html5 to validate the term-table page #15577

octopusinvitro commented Sep 26, 2016 •

edited

Loading

tmtmtmtm left a comment

tmtmtmtm Sep 26, 2016

tmtmtmtm Sep 26, 2016

octopusinvitro Sep 26, 2016

tmtmtmtm Sep 26, 2016

octopusinvitro Sep 26, 2016

tmtmtmtm Sep 26, 2016

tmtmtmtm Sep 26, 2016

octopusinvitro Sep 26, 2016

octopusinvitro commented Sep 27, 2016

tmtmtmtm Sep 28, 2016

tmtmtmtm commented Sep 28, 2016

octopusinvitro commented Oct 11, 2016

octopusinvitro commented Oct 14, 2016

Use tidy-html5 to validate the term-table page #15577

Use tidy-html5 to validate the term-table page #15577

Conversation

octopusinvitro commented Sep 26, 2016 • edited Loading

What does this do?

Why was this needed?

Relevant Issue(s)

Implementation notes

Screenshots

Notes to Reviewer

Notes to Merger

tmtmtmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octopusinvitro commented Sep 27, 2016

Choose a reason for hiding this comment

tmtmtmtm commented Sep 28, 2016

octopusinvitro commented Oct 11, 2016

octopusinvitro commented Oct 14, 2016

octopusinvitro commented Sep 26, 2016 •

edited

Loading