Please include a before / after screenshot :)

Original form screenshot:

New version with this patch:

Thanks. This looks so good. @samueldr want to take a look?

HTML attributes in the template should be consistently quoted using double quotes unless the value includes them. (Yeah, I know the template mixes them :/ already).

These two lines were literally cut-and-paste, and while I am surprised (and saddened) to be criticized for following the existing conventions of lines immediately before and after my changes, I have changed the quoting style as requested.

Delta icon

The SVG you supplied was a black filled triangle, not an uppercase Greek Delta character (the convention for indicating change) so I have modified it to supply the latter in the update commit.

Stacking headers
The way you have stacked the Jobs label on top of the checkmark seems to work, but this gives the first column a bit more width compared to the next two.

I had not visibly observed the difference, but you are correct, there was a 5.47 pixel difference between the column widths, so I increased the column sizes and they are now reporting identically.

The last one is given less width because of its missing width: 2em style. (Though this comment is only valid when the columns all have digits with the same width.)

I have not given the final column a width because there is nothing to the right of it that would be affected by its width in the layout process, and there are several other columns (for this and other tables) which do not supply a width, leading me to believe that the width was a subjective addition for layout aesthetic purposes. If there is another intention here I am unaware of, please feel free to add the width styling.

The fix would be to add another on top of the table, with an initial empty with a variable colspan ...

My initial implementation used this approach, but the standard table styling added a separator line between the two header columns, and the result looked less appealing, because "Jobs" was then floating by itself in a row with a visual separator from the following row which contained the other headers, including those that it referenced. While style overrides could be specified, the existing solution seemed simpler.

It may be that to be entirely semantically correct, we'd prefer to use rowspan on every columns except those with jobs, if you're unsure how to test it that way, I can look into providing a diff with it.

It's not clear to me what this would achieve, so perhaps I am not understanding your suggestion. If you are requesting something like:

<th>
  <td rowspan="2" align="bottom">#</td>
  <td rowspan="2" align="bottom">Jobset</td>
  <td rowspan="2" align="bottom">Date</td>
  <td rowspan="2" align="bottom">Input changes</td>
  <td rowspan="2">Jobs</td>
  <td rowspan="2" align="bottom"><img .../></td>
  <td rowspan="2" align="bottom"><img .../></td>
  <td rowspan="2" align="bottom"><img .../></td>
</th>
<th>
  <td><img ...></td>
</th>

then it seems like the existing:

<th>
  <td>#</td>
  <td>Jobset</td>
  <td>Date</td>
  <td>Input changes</td>
  <td>Jobs<br><img .../></td>
  <td><img .../></td>
  <td><img .../></td>
  <td><img .../></td>
</th>

provides the identical visual representation but it is easier for both the layout engine and the user maintenance of this code.

Alternatively the word "Jobs" could simply be removed, but since I find myself repeatedly explaining to users what these columns indicate, it seemed that the addition of this simple term would improve the understandability of this table for newer users.

These two lines were literally cut-and-paste, and while I am surprised (and saddened) to be criticized for following the existing conventions of lines immediately before and after my changes, I have changed the quoting style as requested.

Yeah, sorry :/ I'm quite new to this project here and didn't really know whether to nit or not on that.

The SVG you supplied was a black filled triangle, not an uppercase Greek Delta character (the convention for indicating change) so I have modified it to supply the latter in the update commit.

Yeah you're right here. I was more concerned into finding an existing glyph in the existing collection than actually concerned with the meaning behind it. Looks good.

I have not given the final column a width because there is nothing to the right of it that would be affected by its width in the layout process, and there are several other columns (for this and other tables) which do not supply a width, leading me to believe that the width was a subjective addition for layout aesthetic purposes. If there is another intention here I am unaware of, please feel free to add the width styling.

Right, I tried being brief in my review, and the intent behind the sentence was lost. I meant: "and if anyone wonders about the last column, it's not important whether it's the same width or not, and it's not right now because of how it doesn't have a width". Nothing has to be changed.

Stacked headers

Here's the end-result when stacking the headers using rowspan.

Yes, there was an annoying border added by bootstrap's styles. Here's the diff I used to achieve this result.

diff --git a/src/root/common.tt b/src/root/common.tt
index 963c806d..6ebeb217 100644
--- a/src/root/common.tt
+++ b/src/root/common.tt
@@ -408,25 +408,27 @@ BLOCK renderEvals %]
   <table class="table table-condensed table-striped clickable-rows">
     <thead>
       <tr>
-        <th>#</th>
+        <th rowspan="2">#</th>
         [% IF !jobset && !build %]
-          <th>Jobset</th>
+          <th rowspan="2">Jobset</th>
         [% END %]
-        <th style="width: 10em">Date</th>
-        <th>Input changes</th>
-        <th style="width: 2.5em">
-        Jobs<br>
+        <th rowspan="2" style="width: 10em">Date</th>
+        <th rowspan="2">Input changes</th>
+        <th colspan="3">Jobs</th>
+        <th rowspan="2">
+            <img src="[% c.uri_for("/static/images/delta.svg") %]" height="12" width="12" title="Delta" alt="Delta" class="build-status" />
+        </th>
+      </tr>
+      <tr>
+        <th style="width: 2em">
             <img src="[% c.uri_for("/static/images/emojione-check-2714.svg") %]" height="12" width="12" title="Succeeded" alt="Succeeded" class="build-status" />
         </th>
-        <th style="width: 2.5em">
+        <th style="width: 2em">
             <img src="[% c.uri_for("/static/images/emojione-red-x-274c.svg") %]" height="12" width="12" title="Failed" alt="Failed" class="build-status" />
         </th>
-        <th style="width: 2.5em">
+        <th style="width: 2em">
             <img src="[% c.uri_for("/static/images/emojione-question-2754.svg") %]" height="12" width="12" title="Queued" alt="Queued" class="build-status" />
         </th>
-        <th>
-            <img src="[% c.uri_for("/static/images/delta.svg") %]" height="12" width="12" title="Delta" alt="Delta" class="build-status" />
-        </th>
       </tr>
     </thead>
     <tbody>
diff --git a/src/root/static/css/hydra.css b/src/root/static/css/hydra.css
index 9fe6e651..7eebc588 100644
--- a/src/root/static/css/hydra.css
+++ b/src/root/static/css/hydra.css
@@ -157,3 +157,8 @@ td.product-icon img {
 	width: 16px;
 	height: 16px;
 }
+
+/* Assumes borders are never desired in multi-row thead. */
+.table thead th {
+	border-top: 0;
+}

The first part of my suggestion, in hindsight, wasn't good at all.

The fix would be to add another on top of the table, with an initial empty with a variable colspan ...

This is what I meant with rowspans; the first row builds the usual <th> with rowspan="2", except for Jobs which will stay on one row, spanning three columns. The next row has columns already "existing" where the preceding one have rowspan, so adding only three columns put them at the right location.

Only as an example, I put back width: 2em to the columns.

I understand about being new and wanting to improve the consistency of the code . I agree with your ultimate intent, although I think that is probably better accomplished as a pass of its own instead of addressing those concerns on individual PR's.

Thanks for the clarification on the rowspan-based solution. I've updated the PR with this approach.

I do appreciate the review and suggestions, thank you!

@samueldr , @grahamc Is there anything else I should do to get this merged to master?

Thank you!