Merge lp:~jcsackett/launchpad/mergeproposal-status-icon-589584 into lp:launchpad

[j.c.sackett]

[r=allenap,bac][ui=none][bug=589584] Updates the clickable status text in an MP to show that it's clickable by changing the cursor on hovers.

Summary
=======

On a merge proposal (for example, this one), there is both an edit icon link and a colored text section indicating the MP's status. Both are clickable and allow you to set the MP status (assuming you have permission to do so) but the mouse cursor only indicates that the edit icon is clickable.

This branch sets an anchor on the text as well, so it properly indicates that it's a control.

Implementation
==============

lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt
------------------------------------------------------------

The span that contained the MP status text is now set only to display if the edit link is not enabled. If the link is enabled, an anchor with similar css class as the span is show instead. The span, new anchor, and the anchor around the edit icon are now inside a tal block defining the link variable, since all three elements require it.

Demo & QA
=========

Create an MP on launchpad.dev; when you hover the mouse over the status text, it should change to the cursor type indicating a clickable control (this is usually a hand). Clicking the link or the edit icon should allow you to set the status.

Lint
====

$ make lint

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt

[Gavin Panella (allenap)]

Nice attention to detail.

I've spotted several problems though, and in the course of reviewing
I've come up with an alternative diff. I hope you don't mind. I've put
it at the end if you're interested.

[1]

+ <a tal:condition="link/enabled"
+ tal:attributes="class string:value mergestatus${context/queue_status/name};
+ href link/url">
+ Work in progress
+ </a>

This will always display "Work in progress"... I think a tal:content
is missing :)

[2]

         <a tal:define="link context/menu:context/edit_status"
- tal:condition="link/enabled"
            tal:attributes="href link/url">
            <img class="editicon" src="/@@/edit" alt="Edit status" />
         </a>

This means that the edit status link will always be visible. It can be
clicked by anonymous users, who are asked to log-in, but if they don't
have permission to edit the proposal status once logged-in I suspect
Launchpad will return a 403 Forbidden.

It's probably safest to just do what link/enabled dictates, and hide
the link if it's False.

[3]

         <a tal:define="link context/menu:context/edit_status"

You could use the link variable defined in tal:merge-status.

[4]

+ <tal:merge-status define="link context/menu:context/edit_status">

You could use the context_menu defined earlier in the template.

My diff:

=== modified file 'lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2010-03-15 20:28:52 +0000
+++ lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2010-12-23 11:24:23 +0000
@@ -17,15 +17,20 @@
     <tr id="summary-row-1-status">
       <th>Status:</th>
       <td id="branchmergeproposal-status-value">
- <span tal:attributes="class string:value mergestatus${context/queue_status/name}"
- tal:content="context/queue_status/title">
- Work in progress
- </span>
- <a tal:define="link context/menu:context/edit_status"
- tal:condition="link/enabled"
- tal:attributes="href link/url">
- <img class="editicon" src="/@@/edit" alt="Edit status" />
- </a>
+ <tal:merge-status define="link context_menu/edit_status">
+ <a tal:omit-tag="not:link/enabled"
+ tal:attributes="href link/url">
+ <span tal:content="context/queue_status/title"
+ tal:define="status_name context/queue_status/name"
+ tal:attributes="class string:value mergestatus${status_name}">
+ Work in progress
+ </span>
+ </a>
+ <a tal:condition="link/enabled"
+ tal:attributes="href link/url">
+ <img class="editicon" src="/@@/edit" alt="Edit status" />
+ </a>
+ </tal:merge-status>
       </td>
     </tr>
     <tal:comment condition="nothing">

review: Needs Fixing

[j.c.sackett (jcsackett)]

Gavin--

This is what I get for cargo culting when tired.

Your diff does seem to be cleaner, the only problem is that the link underline on hover isn't the same color as the text in the status. To be consistent with other elements of Launchpad, the underline should match in color, rather than being the default blue of a link.

I'll modify your diff to get that effect, and push up the changes.

[Gavin Panella (allenap)]

Jon, please add a comment when you update this so that I don't miss it. Thanks :)

[j.c.sackett (jcsackett)]

Gavin--

I've pushed up changes; in the end I basically had to just fix my diff, since the coloration issue won't work with a span inside the anchor.

I also had to fix up the javascript related to the spinner and color changes as status is changed; you should see the change in the diff, but essentially I updated it to look for an anchor tag where it was looking for spans.

[Brad Crittenden (bac)]

Other than fixing the indentation on your final <a> I think this looks good.

review: Approve (code)

[Gavin Panella (allenap)]

Looks good :)

review: Approve

=== modified file 'lib/lp/code/javascript/branchmergeproposal.status.js'
--- lib/lp/code/javascript/branchmergeproposal.status.js	2010-07-15 10:55:27 +0000
+++ lib/lp/code/javascript/branchmergeproposal.status.js	2011-01-04 00:18:41 +0000
@@ -33,9 +33,9 @@
                         var cb = status_choice_edit.get('contentBox');
                         Y.Array.each(conf.status_widget_items, function(item) {
                                 if (item.value == status_choice_edit.get('value')) {
-                                    cb.query('span').addClass(item.css_class);
+                                    cb.one('a').addClass(item.css_class);
                                 } else {
-                                    cb.query('span').removeClass(item.css_class);
+                                    cb.one('a').removeClass(item.css_class);
                                 }
                             });
                         update_summary();

=== modified file 'lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt	2010-03-15 20:28:52 +0000
+++ lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt	2011-01-04 00:18:41 +0000
@@ -17,15 +17,25 @@
     <tr id="summary-row-1-status">
       <th>Status:</th>
       <td id="branchmergeproposal-status-value">
-        <span tal:attributes="class string:value mergestatus${context/queue_status/name}"
-              tal:content="context/queue_status/title">
-          Work in progress
-        </span>
-        <a tal:define="link context/menu:context/edit_status"
-           tal:condition="link/enabled"
-           tal:attributes="href link/url">
-           <img class="editicon" src="/@@/edit" alt="Edit status" />
-        </a>
+
+        <tal:merge-status define="link context_menu/edit_status;
+                                  status_name context/queue_status/name">
+          <a tal:condition="link/enabled"
+             tal:content="context/queue_status/title"
+             tal:attributes="href link/url;
+                             class string:value mergestatus${status_name}">
+            Work in progress
+          </a>
+          <span tal:condition="not: link/enabled"
+                tal:content="context/queue_status/title"
+                tal:attributes="class string:value mergestatus${status_name}">
+            Work in progress
+          </span>
+            <a tal:condition="link/enabled"
+               tal:attributes="href link/url">
+              <img class="editicon" src="/@@/edit" alt="Edit status" />
+            </a>
+        </tal:merge-status>
       </td>
     </tr>
     <tal:comment condition="nothing">