Test page markup is (still a little) weird/busted

[ry5n]

Multiple h1 tags without sectioning wrappers:

<h1 id="main">Vellum example page</h1>
…
<h1 id="swatches">Colors</h1>
…
<h1 id="headings">Headings</h1>

Ids are snake_case; normally we write kebab-case in HTML:

<label for="text_area">Text Area:</label>
<textarea id="text_area"></textarea>

Fieldset wrapping a form is weird, in this case not really correct (a fieldset should wrap a set of related fields):

<fieldset>
    <p>…</p>
    <form>…</form>
</fieldset>

Labels uses where a fieldset + legend would be appropriate: this is what a fieldset is actually for. (FYI the label wrapping input is also weird but is being fixed in #74.)

<p>
<label for="radio_buttons">Radio Buttons:</label>
<label><input type="radio" class="radio" name="radio_button" value="radio_1" /> Radio 1</label>
…
</p>

I’d give the table markup a C- grade: cellspacing and cellpadding aren’t needed anymore; the first <tr> should be a thead with a tbody after containing the other rows. The even class is also no longer needed. “Division” is not what <td> stands for: it’s “table data”, but this should just say “Cell 1” etc.

<table cellspacing="0" cellpadding="0">
    <tr>
      <th>Header 1</th><th>Header 2</th><th>Header 3</th>
    </tr>
    <tr>
      <td>Division 1</td><td>Division 2</td><td>Division 3</td>
    </tr>
    <tr class="even">
      <td>Division 1</td><td>Division 2</td><td>Division 3</td>
    </tr>
    <tr>
      <td>Division 1</td><td>Division 2</td><td>Division 3</td>
    </tr>
  </table>

This also seems strange but not sure if it’s strictly wrong.

<pre><p>…</p></pre>

[jeffkamo]

I don't think this should've been closed prior to your PR being merged.

[ry5n]

That PR fixed some of these but not all. multiple H1s still just chillin’ there.

[nastiatikk]

Actually this is weird… It had sections as I remember. Will explore this issue

[nastiatikk]

@ry5n https://github.com/mobify/vellum/pull/87/files This PR has different content

[nastiatikk]

@jeffkamo @ryan if PR was merged and PR was based on this issue why this issue should not be closed?

In addition all the changes from ^ this PR disappeared in updated master branch. https://github.com/mobify/vellum/blob/master/test/index.html

[nastiatikk]

Maybe it came as a result of https://github.com/mobify/vellum/pull/74 merge…

[jeffkamo]

Ultimately, if the code is fixed in the master branch, the issue can be closed. Although if the issue has not been fixed, this issue should remain open, imo.

[nastiatikk]

Oh, my bad… I thought it was a comment after the branch was merged, but it was before.

[nastiatikk]

https://github.com/mobify/vellum/commit/80c555c58f4942157a86fc321d2a975e5ea6fa09 sorry @ry5n =(

[ry5n]

Yup multiple h1 elements still hanging out together in the same sectioning level. We really shouldn’t use more than one h1 per page at all, screen readers don’t support that. So we need to fix the headings at least. The other things mentioned in the issue summary should be checked for and fixed as well.

[nastiatikk]

@ry5n yes, but it came as a result of your merge https://github.com/mobify/vellum/commit/80c555c58f4942157a86fc321d2a975e5ea6fa09 and it was fixed before, the whole index.html structure was changed

[ry5n]

Ooooooh OK, I see. Sorry. I can fix it, I just don’t have time at the moment.

[nastiatikk]

No problem. I can fix it myself, but was not sure that it's good, because this change was involved to other branches.

[ry5n]

Well, let’s reintroduce the fixes here and it should be a quick review.

[nastiatikk]

Here? But this is an Issue, not PR…

The fix will look like replacing index.html with the previous version of this file

[ry5n]

@nastiatikk Haha, you’re right of course :). Sounds like a plan.