Merge lp:~deryck/launchpad/icon-positioning-js-400057 into lp:launchpad

[Deryck Hodge]

[Deryck Hodge (deryck)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

= Summary =

After the addition of r8886 in devel, the add/remove icons are set via
sprite classes and positioned differently than when the JavaScript for
handling inline subscribing was originally written. So when subscribing
inline the icons are positioned incorrectly currently. This problem is
captured in bug 400057.

== Proposed fix ==

The correct fix is to get have the js that generates HTML use sprite
classes instead of inserting images.

== Pre-implementation notes ==

I didn't have any specific pre-imp discussions but plenty of discussion
happened on bug 383555 and bug 401658.

== Implementation details ==

This fix is pretty simple, just matching the current UI classes in js.
There are some outstanding questions about alignment from mpt in bug 401658.

== Tests ==

A Windmill test has been updated, and can be run with:

./bin/lp-windmill -e
test=lib/lp/bugs/windmill/tests/test_bugs/test_bug_inline_subscriber.py
firefox http://bugs.launchpad.dev:8085

== Demo and Q/A ==

Subscribe and unsubscribe yourself a couple times and note that the
remove icon should appear to the right of the "Unsubscribe" link and
should be flush with the link. The add icon should always be to the
left of the link.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/javascript/bugs/subscriber.js
  lib/canonical/launchpad/javascript/bugs/bugtask-index.js
  lib/lp/bugs/windmill/tests/test_bugs/test_bug_inline_subscriber.py

== JSLint notices ==
jslint: Lint found in
'/home/deryck/canonical/lp-branches/icon-positioning-js-400057/lib/canonical/launchpad/javascript/bugs/bugtask-index.js':
Line 1263 character 58: Missing semicolon.
        status_content.on('click', function(e) { e.halt() });

Line 1264 character 62: Missing semicolon.
        importance_content.on('click', function(e) { e.halt() });

jslint: No problem found in
'/home/deryck/canonical/lp-branches/icon-positioning-js-400057/lib/canonical/launchpad/javascript/bugs/subscriber.js'.

jslint: 2 files to lint.

(Note: These lint notices are fixed in another, not yet landed, branch.)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpp+FkACgkQ4glRK0DaE8j69ACgt9PkUAVbefqAgoZMthVAwuTg
B9IAoK2TEaVe6IAfg1L76ulTBQF0gGA+
=MhGd
-----END PGP SIGNATURE-----

[Guilherme Salgado (salgado)]

Looks good to me

review: Approve

=== modified file 'lib/canonical/launchpad/javascript/bugs/bugtask-index.js'
--- lib/canonical/launchpad/javascript/bugs/bugtask-index.js	2009-07-17 18:46:25 +0000
+++ lib/canonical/launchpad/javascript/bugs/bugtask-index.js	2009-07-24 17:21:52 +0000
@@ -714,21 +714,23 @@
         terms.subscribed_by = 'by ' + full_name;
     }
 
-    if (subscription.is_team()) {
-        terms.img_url = '/@@/team';
-    } else {
-        terms.img_url = '/@@/person';
-    }
-
-    var html = Y.Node.create(
-        '<div><a><img alt="" width="14" height="14" />' +
-        '&nbsp;</a></div>');
+    var html = Y.Node.create('<div><a></a></div>');
     html.addClass('subscriber-' + terms.name);
-    html.query('img').set('src', terms.img_url);
+
     html.query('a')
         .set('href', '/~' + terms.name)
         .set('name', terms.full_name)
-        .set('title', 'Subscribed ' + terms.subscribed_by)
+        .set('title', 'Subscribed ' + terms.subscribed_by);
+
+    var span;
+    if (subscription.is_team()) {
+        span = '<span class="sprite team"></span>';
+    } else {
+        span = '<span class="sprite person"></span>';
+    }
+
+    html.query('a')
+        .appendChild(Y.Node.create(span))
         .appendChild(document.createTextNode(terms.display_name));
 
     // Add remove icon if the current user can unsubscribe the subscriber.
@@ -1079,8 +1081,8 @@
                             subscription.set('has_dupes', true);
                         }
                     }
-                    subscription_link.setStyle('background',
-                        'url(/@@/add) left center no-repeat');
+                    subscription_link.removeClass('sprite-after remove');
+                    subscription_link.addClass('sprite add');
                 });
                 anim.run();
             },
@@ -1125,8 +1127,6 @@
     var config = {
         on: {
             success: function(client) {
-                subscription_link.setStyle('background',
-                    'url(/@@/remove) left center no-repeat');
                 subscription.disable_spinner('Unsubscribe');
 
                 if (subscription.has_duplicate_subscriptions()) {

=== modified file 'lib/canonical/launchpad/javascript/bugs/subscriber.js'
--- lib/canonical/launchpad/javascript/bugs/subscriber.js	2009-07-08 20:42:19 +0000
+++ lib/canonical/launchpad/javascript/bugs/subscriber.js	2009-07-24 17:21:52 +0000
@@ -163,17 +163,18 @@
      */
     'disable_spinner': function(text) {
         if (Y.Lang.isValue(text)) {
-            this.get('link').set('innerHTML', text);
+            var link = this.get('link');
+            link.set('innerHTML', text);
             if (text == 'Subscribe') {
-                this.get('link').setStyle('background',
-                    'url(/@@/add) left center no-repeat');
+                link.removeClass('sprite-after remove');
+                link.addClass('sprite add');
             } else {
-                this.get('link').setStyle('background',
-                    'url(/@@/remove) left center no-repeat');
+                link.removeClass('sprite add');
+                link.addClass('sprite-after remove');
             }
         }
         this.get('spinner').setStyle('display', 'none');
-        this.get('link').setStyle('display', 'block');
+        this.get('link').setStyle('display', 'inline');
     }
 });
 

=== modified file 'lib/lp/bugs/windmill/tests/test_bugs/test_bug_inline_subscriber.py'
--- lib/lp/bugs/windmill/tests/test_bugs/test_bug_inline_subscriber.py	2009-07-17 00:26:05 +0000
+++ lib/lp/bugs/windmill/tests/test_bugs/test_bug_inline_subscriber.py	2009-07-24 17:50:13 +0000
@@ -45,7 +45,7 @@
     client.asserts.assertNode(classname=SAMPLE_PERSON_CLASS)
     client.asserts.assertProperty(
         xpath=SUBSCRIPTION_LINK,
-        validator=u'style.backgroundImage|url(/@@/remove)')
+        validator=u'className|remove')
 
     # Make sure the unsubscribe link also works, that
     # the person's named is removed from the subscriber's list,
@@ -56,7 +56,7 @@
         xpath=SUBSCRIPTION_LINK, validator=u'Subscribe')
     client.asserts.assertProperty(
         xpath=SUBSCRIPTION_LINK,
-        validator=u'style.backgroundImage|url(/@@/add)')
+        validator=u'className|add')
     client.asserts.assertNotNode(classname=SAMPLE_PERSON_CLASS)
 
     # Subscribe again in order to check that the minus icon
@@ -71,7 +71,7 @@
         xpath=SUBSCRIPTION_LINK, validator=u'Subscribe')
     client.asserts.assertProperty(
         xpath=SUBSCRIPTION_LINK,
-        validator=u'style.backgroundImage|url(/@@/add)')
+        validator=u'className|add')
     client.asserts.assertNotNode(classname=SAMPLE_PERSON_CLASS)
 
     # Test inline subscribing of others by subscribing Ubuntu Team.