launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/bug-nav-links-924801 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #924801 in Launchpad itself: ""First • Previous • Next • Last" links are active when they don't do anything"
  https://bugs.launchpad.net/launchpad/+bug/924801

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bug-nav-links-924801/+merge/123872

== Implementation ==

After much experimentation, the easiest thing to do here was to update some existing rules - "invalid-link" has been used with disabled anchors, so :hover and :selected pseudo classes were added so that the links appear fully disabled. This change ensures the cursor doesn't change to a pointer and the link stays grey etc when hovered over or clicked. The click handler for the links was updated to check for the 'invalid-link' class and return if set.

A bot of refactoring was also done to eliminate the need for the namespace method "linkify_navigation". It is better that this be an internal method which is called by the widget itself rather than requiring the callsites to invoke it.

== Tests ==

Update the test_listing_navigator yui tests.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/icing/css/typography.css
  lib/lp/app/javascript/listing_navigator.js
  lib/lp/app/javascript/tests/test_listing_navigator.js
  lib/lp/bugs/javascript/buglisting.js
  lib/lp/registry/javascript/sharing/granteetable.js
-- 
https://code.launchpad.net/~wallyworld/launchpad/bug-nav-links-924801/+merge/123872
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bug-nav-links-924801 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/icing/css/typography.css'
--- lib/canonical/launchpad/icing/css/typography.css	2012-06-15 18:21:34 +0000
+++ lib/canonical/launchpad/icing/css/typography.css	2012-09-12 02:03:23 +0000
@@ -77,8 +77,7 @@
 a.help.icon, a.sprite.maybe.help {
     border: none;
 }
-a.invalid-link {
-    disabled: True;
+a.invalid-link, a.invalid-link:active, a.invalid-link:hover {
     color: #909090;
     text-decoration: none;
     cursor: default;

=== modified file 'lib/lp/app/javascript/listing_navigator.js'
--- lib/lp/app/javascript/listing_navigator.js	2012-07-18 14:08:32 +0000
+++ lib/lp/app/javascript/listing_navigator.js	2012-09-12 02:03:23 +0000
@@ -16,6 +16,21 @@
 }
 
 /**
+ * Rewrite all nodes with navigation classes so that they are hyperlinks.
+ * Content is retained.
+ */
+function linkify_navigation(container_node) {
+    Y.each(['previous', 'next', 'first', 'last'], function(class_name) {
+        container_node.all('.' + class_name).each(function(node) {
+            var new_node = Y.Node.create('<a href="#"></a>');
+            new_node.addClass(class_name);
+            new_node.setContent(node.getContent());
+            node.replace(new_node);
+        });
+    });
+}
+
+/**
  * If there is no context (say /bugs/+bugs) we need to generate a weblink for
  * the lp.client to use for things. This is a helper to take the current url
  * and try to generate a likely web_link value to build a url off of.
@@ -110,6 +125,15 @@
     pre_fetch: {value: false},
     navigation_indices: {valueFn: empty_nodelist},
     target: {value: null},
+    container: {
+        value: null,
+        getter: function(value) {
+            if (!Y.Lang.isValue(value)) {
+                return this.get('target');
+            }
+            return value;
+        }
+    },
     template: {value: null}
 };
 
@@ -149,6 +173,7 @@
             this.get('model').get('history').after(
                 'change', this.default_history_changed, this);
         }
+        linkify_navigation(this.get('container'));
     },
 
     /**
@@ -189,7 +214,15 @@
     clickAction: function(selector, callback) {
         var nodes = this.get('target').get('parentNode').all(selector);
         nodes.on('click', function(e) {
-            e.preventDefault();
+            e.halt();
+            // If the target link is disabled, we want to ignore the click.
+            var link = e.target;
+            if (link.get('tagName').toLowerCase() !== 'a' ) {
+                link = link.ancestor('a');
+            }
+            if (!Y.Lang.isValue(link) || link.hasClass('invalid-link')) {
+                return;
+            }
             callback.call(this);
         }, this);
         nodes.addClass('js-action');
@@ -307,9 +340,9 @@
      */
     update_navigation_links: function() {
         this.get('backwards_navigation').toggleClass(
-            'inactive', !this.has_prev());
+            'invalid-link', !this.has_prev());
         this.get('forwards_navigation').toggleClass(
-            'inactive', !this.has_next());
+            'invalid-link', !this.has_next());
     },
 
     update_from_new_model: function(query, fetch_only, model) {
@@ -519,22 +552,6 @@
 
 
 /**
- * Rewrite all nodes with navigation classes so that they are hyperlinks.
- * Content is retained.
- */
-module.linkify_navigation = function(container_node) {
-    Y.each(['previous', 'next', 'first', 'last'], function(class_name) {
-        container_node.all('.' + class_name).each(function(node) {
-            new_node = Y.Node.create('<a href="#"></a>');
-            new_node.addClass(class_name);
-            new_node.setContent(node.getContent());
-            node.replace(new_node);
-        });
-    });
-};
-
-
-/**
  * Return the value for a given feature flag in the current scope.
  * Only flags declared as "related_features" on the view are available.
  */

=== modified file 'lib/lp/app/javascript/tests/test_listing_navigator.js'
--- lib/lp/app/javascript/tests/test_listing_navigator.js	2012-09-10 21:02:05 +0000
+++ lib/lp/app/javascript/tests/test_listing_navigator.js	2012-09-12 02:03:23 +0000
@@ -88,7 +88,7 @@
             Y.lp.testing.helpers.reset_history();
         },
 
-        get_render_navigator: function() {
+        get_render_navigator: function(container) {
             var lp_cache = {
                 mustache_model: {
                     items: [{foo: 'bar', show_foo: true}]
@@ -105,7 +105,8 @@
             var navigator =  new TestListingNavigator({
                 cache: lp_cache,
                 template: template,
-                target: this.target
+                target: this.target,
+                container: container
             });
             var index = Y.Node.create(
                 '<div><strong>3</strong> &rarr; <strong>4</strong>' +
@@ -132,7 +133,7 @@
             var navigator = this.get_render_navigator();
             var action = navigator.get('backwards_navigation').item(0);
             navigator.update_navigation_links();
-            Y.Assert.isTrue(action.hasClass('inactive'));
+            Y.Assert.isTrue(action.hasClass('invalid-link'));
         },
         /**
          * update_navigation_links should enable "previous" and "first" if
@@ -147,7 +148,7 @@
                 start: 1, memo: 'pi'
             };
             navigator.update_navigation_links();
-            Y.Assert.isFalse(action.hasClass('inactive'));
+            Y.Assert.isFalse(action.hasClass('invalid-link'));
         },
         /**
          * update_navigation_links should disable "next" and "last" if there is
@@ -158,7 +159,7 @@
             var navigator = this.get_render_navigator();
             var action = navigator.get('forwards_navigation').item(0);
             navigator.update_navigation_links();
-            Y.Assert.isTrue(action.hasClass('inactive'));
+            Y.Assert.isTrue(action.hasClass('invalid-link'));
         },
         /**
          * update_navigation_links should enable "next" and "last" if there is a
@@ -173,10 +174,10 @@
                 start: 1, memo: 'pi'
             };
             navigator.update_navigation_links();
-            Y.Assert.isFalse(action.hasClass('inactive'));
+            Y.Assert.isFalse(action.hasClass('invalid-link'));
         },
         /**
-         * linkify_navigation should convert previous, next, first last into
+         * Creating a navigator should convert previous, next, first last into
          * hyperlinks, while retaining the original content.
          */
         test_linkify_navigation: function() {
@@ -188,11 +189,12 @@
                 '<span class="first">FiRST</span>' +
                 '<span class="last">lAst</span>' +
                 '</div>');
-            module.linkify_navigation(Y.one('#bugs-table-listing'));
+            this.target = Y.one('#bugs-table-listing');
+            this.get_render_navigator(this.target);
             function checkNav(selector, content) {
                 var nodelist = Y.all(selector);
                 nodelist.each(function (node) {
-                    if (node.get('id') != 'notme') {
+                    if (node.get('id') !== 'notme') {
                         Y.Assert.areEqual('a',
                             node.get('tagName').toLowerCase());
                         Y.Assert.areEqual(content, node.getContent());
@@ -234,7 +236,6 @@
             this.target = Y.Node.create('<div></div>').set(
                 'id', 'client-listing');
             Y.one('body').appendChild(this.target);
-            console.log('setup', Y.config.win.history.state);
             Y.config.win.history.state = {};
         },
 

=== modified file 'lib/lp/bugs/javascript/buglisting.js'
--- lib/lp/bugs/javascript/buglisting.js	2012-07-24 16:06:11 +0000
+++ lib/lp/bugs/javascript/buglisting.js	2012-09-12 02:03:23 +0000
@@ -161,12 +161,12 @@
         var navigation_indices = Y.all('.batch-navigation-index');
         var pre_fetch = Y.lp.app.listing_navigator.get_feature_flag(
             'bugs.dynamic_bug_listings.pre_fetch');
-        Y.lp.app.listing_navigator.linkify_navigation(container);
         var navigator = new module.BugListingNavigator({
             current_url: window.location,
             cache: LP.cache,
             template: LP.mustache_listings,
             target: target,
+            container: container,
             navigation_indices: navigation_indices,
             pre_fetch: Boolean(pre_fetch)
         });

=== modified file 'lib/lp/registry/javascript/sharing/granteetable.js'
--- lib/lp/registry/javascript/sharing/granteetable.js	2012-09-06 14:42:26 +0000
+++ lib/lp/registry/javascript/sharing/granteetable.js	2012-09-12 02:03:23 +0000
@@ -110,7 +110,6 @@
         var target = Y.one('#grantee-table');
         var container = target.get('parentNode');
         var navigation_indices = Y.all('.batch-navigation-index');
-        Y.lp.app.listing_navigator.linkify_navigation(target);
         var ns = Y.lp.registry.sharing.granteelisting_navigator;
         var cache = LP.cache;
         cache.total = this.get('grantees').length;
@@ -118,6 +117,7 @@
             current_url: window.location,
             cache: cache,
             target: target,
+            container: container,
             navigation_indices: navigation_indices,
             batch_info_template: '<div></div>'
         });