575211 - show_bug.cgi should use field-label.html.tmpl for field headers

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement, P1)

Description

[Reed Loden [:reed]]

Attachment: wip

[apologies if there's a dupe of this... I looked and couldn't find anything that matched exactly]

show_bug.cgi (edit.html.tmpl) should use field-lable.html.tmpl for field headers. By doing this, users automagically get title-based help and better linkification to documentation, plus it makes it easier to eventually move to field.html.tmpl as fields are added.

Comment 1

[Reed Loden [:reed]]

Attachment: patch - v1

This works, but I still might want to edit a few things...

Comment 2

[Max Kanat-Alexander]

This is going to have to be a 4.2 thing for now--it's too close to the freeze and this patch is really large.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 454476 [details] [diff] [review]
patch - v1

>=== modified file 'template/en/default/bug/edit.html.tmpl'
>           <tr>
>           [% IF bug.check_can_change_field('alias', 0, 1) %]      
>+            [% INCLUDE "bug/field-label.html.tmpl"
>+               field = bug_fields.alias
>+               editable = bug.check_can_change_field("alias", 0, 1)
>+            %]
>             <td>

  I think you're adding an extra column there. I'm not sure a straightforward replacement is possible for this field.

>@@ -349,16 +350,17 @@
>     [%# PLATFORM #%]
>     [%############%]    
>     <tr>
>-      <td class="field_label">
>-        <label for="rep_platform" accesskey="h"><b>Platform</b></label>:
>-      </td>
>+      <th class="field_label">
>+        <label for="rep_platform" accesskey="i">
>+          <b>Platform</b></label>:
>+      </th>

  Why not use field-label there?

>@@ -408,17 +411,17 @@
>+      <th class="field_label">
>         <label for="priority" accesskey="i">
>-          <b><a href="page.cgi?id=fields.html#importance"><u>I</u>mportance</a></b></label>:
>-      </td>
>+          <b>Importance</b></label>:
>+      </th>

  Why'd you make that stop being a link?

  Also, if it's a <th> now, it doesn't need the <b> tags anymore.

>@@ -533,16 +537,14 @@
>+    [% IF bug.bug_file_loc AND NOT bug.bug_file_loc.match("^(javascript|data)") %]
>+       bug_file_loc_url = bug.bug_file_loc
>+    [% END %]

  That's text in the page instead of a template directive, inside there.

  Also, because of the way TT works, it should probably have an ELSE that sets it to ''.

>@@ -726,18 +730,18 @@
>-    <td class="field_label">
>+    <th class="field_label">
>       <b>Reported</b>:
>-    </td>
>+    </th>

  Nit: Let's take out the <b> there if we're moving to th.

>+    <th class="field_label">
>+      <b>Modified</b>:
>+    </th>

  And there. It can probably even all be on one line, since it's short.

>@@ -751,9 +755,9 @@
>+      <th class="field_label">
>+        <label for="newcc" accesskey="a"><a href="page.cgi?id=fields.html#cc"><b>CC List</b></a>:</label>
>+      </th>

  Hmmm, I wonder, should we just use field-label and take the name as "CC" instead of "CC List"?

>+      <th class="field_label flags_label">
>+        <label><b>Flags</b>:</label>
>+      </th>

  You can use the "flagtypes.name" field for that, I think. Not sure how to get flags_label in there, but perhaps we should just use #field_label_flagtypes_name in our CSS instead.

  Note that this will require a modification of the "id" to use either css_class_quote or a new filter, perhaps html_id or something like that, that replaces "." (or any invalid character) with "_".


  Everything else looks great! This is a REALLY good cleanup. :-)


  I also noticed while I was reading this patch--in 4.0 we have links to #status, but that doesn't exist in fields.html--it's #bug_status now. That might even be the case for 3.6, too. That probably needs to be fixed as a bug for those branches.

Comment 4

[Max Kanat-Alexander]

Hey reed. Any hope of getting a new patch for this? I'd love to see this get done.

Comment 5

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is too invasive or too risky to be accepted for 4.4 at this point. The target milestone is set to 5.0 which is our next major release.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else on time for 5.0.

Comment 6

[Lalit Khattar]

Attachment: patch-v1.diff

Target milestone and URL are the only two fields that I think could be fitted in bug/field-label html template.

If there are anymore fields to add, please let me know I will add it to the patch.

Thanks,
Lalit

Comment 7

[Frédéric Buclin]

Comment on attachment 8386080 [details] [diff] [review]
patch-v1.diff

>-      <th class="field_label">
>-        <label for="keywords" accesskey="k">
>-          <a href="describekeywords.cgi"><u>K</u>eywords</a></label>:
>-      </th>
>+      [% INCLUDE "bug/field-label.html.tmpl"
>+        field = bug_fields.keywords
>+        editable = 1
>+        accesskey = "k"
>+        desc_url = "describekeywords.cgi"
>+      %]
>       <td class="field_value" colspan="2">
>         [% INCLUDE bug/field.html.tmpl

Instead of calling bug/field-label.html.tmpl and then bug/field.html.tmpl, you should only call bug/field.html.tmpl and remove no_tds. This has the same effect, but is easier to maintain.

Comment 8

[Lalit Khattar]

Attachment: patch-v2.diff

Comment 9

[Frédéric Buclin]

Comment on attachment 8401439 [details] [diff] [review]
patch-v2.diff

>         [% INCLUDE bug/field.html.tmpl
>-           bug = bug, field = bug_fields.keywords, value = bug.keywords
>-           editable = bug.check_can_change_field("keywords", 0, 1),
>-           no_tds = 1, possible_values = all_keywords
>+            bug = bug, field = bug_fields.keywords, value = bug.keywords
>+            editable = bug.check_can_change_field("keywords", 0, 1),
>+            possible_values = all_keywords
>         %]

As discussed on IRC some weeks ago, you must pass desc_url = "describekeywords.cgi", else the label will point to another page by default. Also, now that you removed <td></td>, please fix the indentation (2 whitespaces in templates). Otherwise looks good.

Comment 10

[Lalit Khattar]

Attachment: patch-v3.diff

Two whitespaces indentation corrected and desc_url added

Comment 11

[:glob ✱]

Comment on attachment 8416951 [details] [diff] [review]
patch-v3.diff

Review of attachment 8416951 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

note: this removes the nbsp between 'target' and 'milestone', which impacts layout on narrow windows.
i've filed bug 1020817 to address that

Comment 12

[:glob ✱]

To ssh://[e-mail]/bugzilla/bugzilla.git
   ea10fef..944b327  master -> master