launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/cheap-spinner into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #885272 in Launchpad itself: "Need a loading icon for bug listings ajax navigation and sorting"
  https://bugs.launchpad.net/launchpad/+bug/885272

For more details, see:
https://code.launchpad.net/~abentley/launchpad/cheap-spinner/+merge/83344

= Summary =
Fix bug #885272: Need a loading icon for bug listings ajax navigation and sorting

== Proposed fix ==
Add a hidden spinner at the navigation links and show it when loading.

== Pre-implementation notes ==
Discussed briefly with deryck

== Implementation details ==
The spinners are hidden using the visibility property because they are taller than the navigation text, and if they were display: none, the page would jump when they appeared.

deryck asked me to either complete his branch lp:~deryck/launchpad/buglists-loading-885272 or implement a solution in the style of wallyworld's ajax batch navigator.  I didn't understand how to complete deryck's branch, so I did the latter.

== Tests ==
bin/test -t buglisting.html

== Demo and Q/A ==
Go go a bug listing such as https://bugs.qastaging.launchpad.net/nova/+bugs
Click "Bug number".  A spinner should appear while the new batch is loading.
Click "Next", then "First".  No spinner should appear, because the listings don't need to be loaded.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/javascript/buglisting.js
  lib/lp/bugs/javascript/tests/test_buglisting.js
-- 
https://code.launchpad.net/~abentley/launchpad/cheap-spinner/+merge/83344
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/cheap-spinner into lp:launchpad.

=== modified file 'lib/lp/bugs/javascript/buglisting.js'
--- lib/lp/bugs/javascript/buglisting.js	2011-11-18 20:11:11 +0000
+++ lib/lp/bugs/javascript/buglisting.js	2011-11-24 20:48:27 +0000
@@ -41,7 +41,8 @@
     forwards_navigation: {valueFn: empty_nodelist},
     io_provider: {value: null},
     pre_fetch: {value: false},
-    navigation_indices: {valueFn: empty_nodelist}
+    navigation_indices: {valueFn: empty_nodelist},
+    spinners: {valueFn: empty_nodelist}
 };
 
 
@@ -57,11 +58,6 @@
         delete this.get('search_params').direction;
         delete this.get('search_params').orderby;
         this.set('io_provider', config.io_provider);
-        this.set('error_handler', new Y.lp.client.ErrorHandler());
-        this.get('error_handler').showError = Y.bind(
-            Y.lp.app.errors.display_error, window, null);
-        this.set(
-            'failure_handler', this.get('error_handler').getFailureHandler());
         this.set('field_visibility', cache.field_visibility);
         batch_key = this.handle_new_batch(cache);
         this.set('current_batch', cache);
@@ -72,6 +68,7 @@
             template = template.replace(/\n/g, '
');
         }
         this.set('template', template);
+        this.set_pending(false);
         this.set('target', config.target);
         if (Y.Lang.isValue(config.navigation_indices)) {
             this.set('navigation_indices', config.navigation_indices);
@@ -84,6 +81,17 @@
         this.get('history').on('change', this.history_changed, this);
     },
 
+    get_failure_handler: function(fetch_only){
+        var error_handler = new Y.lp.client.ErrorHandler();
+        error_handler.showError = Y.bind(
+            Y.lp.app.errors.display_error, window, null);
+        if (!fetch_only){
+            error_handler.clearProgressUI = Y.bind(
+                this.set_pending, this, false);
+        }
+        return error_handler.getFailureHandler();
+    },
+
     /**
      * Event handler for history:change events.
      */
@@ -152,6 +160,15 @@
         this.render_navigation();
     },
 
+    set_pending: function(is_pending){
+        if (is_pending){
+            this.get('spinners').setStyle('visibility', 'visible');
+        }
+        else{
+            this.get('spinners').setStyle('visibility', 'hidden');
+        }
+    },
+
     has_prev: function(){
         return !Y.Lang.isNull(this.get('current_batch').prev);
     },
@@ -175,6 +192,7 @@
         if (fetch_only) {
             return;
         }
+        this.set_pending(false);
         this.update_from_cache(query, batch_key);
     },
 
@@ -356,13 +374,16 @@
             on: {
                 success: Y.bind(
                     this.update_from_new_model, this, query, fetch_only),
-                failure: this.get('failure_handler')
+                failure: this.get_failure_handler(fetch_only)
             }
         };
         var context = this.get('current_batch').context;
         if (Y.Lang.isValue(this.get('io_provider'))) {
             load_model_config.io_provider = this.get('io_provider');
         }
+        if (!fetch_only){
+            this.set_pending(true);
+        }
         Y.lp.client.load_model(
             context, '+bugs', load_model_config, query);
     }
@@ -380,6 +401,12 @@
             new_node.addClass(class_name);
             new_node.setContent(node.getContent());
             node.replace(new_node);
+            if (class_name === 'first'){
+                var spinner_node = Y.Node.create(
+                    '<img class="spinner" src="/@@/spinner"'+
+                    ' alt="Loading..." />');
+                new_node.insertBefore(spinner_node, new_node);
+            }
         });
     });
 };
@@ -401,15 +428,16 @@
     var target = Y.one('#client-listing');
     var navigation_indices = Y.all('.batch-navigation-index');
     var pre_fetch = get_feature_flag('bugs.dynamic_bug_listings.pre_fetch');
+    namespace.linkify_navigation();
     var navigator = new namespace.ListingNavigator({
         current_url: window.location,
         cache: LP.cache,
         template: LP.mustache_listings,
         target: target,
         navigation_indices: navigation_indices,
-        pre_fetch: Boolean(pre_fetch)
+        pre_fetch: Boolean(pre_fetch),
+        spinners: Y.all('.spinner')
     });
-    namespace.linkify_navigation();
     navigator.set('backwards_navigation', Y.all('.first,.previous'));
     navigator.set('forwards_navigation', Y.all('.last,.next'));
     navigator.clickAction('.first', navigator.first_batch);

=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting.js'
--- lib/lp/bugs/javascript/tests/test_buglisting.js	2011-11-18 17:20:22 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.js	2011-11-24 20:48:27 +0000
@@ -466,14 +466,18 @@
         lp_cache.prev = null;
     }
     var target = Y.Node.create('<div id="client-listing"></div>');
-    return new module.ListingNavigator({
+    var navigator_config = {
         current_url: url,
         cache: lp_cache,
         io_provider: mock_io,
         pre_fetch: config.pre_fetch,
         target: target,
         template: ''
-    });
+    };
+    if (Y.Lang.isValue(config.spinners)){
+        navigator_config.spinners = config.spinners;
+    }
+    return new module.ListingNavigator(navigator_config);
 };
 
 suite.add(new Y.Test.Case({
@@ -744,6 +748,97 @@
     }
 }));
 
+suite.add(new Y.Test.Case({
+    name: "spinners",
+
+    get_navigator: function() {
+        var spinner = Y.Node.create('<img src="/spinner" />');
+        var navigator = get_navigator(
+            '', {spinners: new Y.NodeList([spinner])});
+        Y.Assert.areSame(navigator.get('spinners').item(0), spinner);
+        return navigator;
+    },
+
+    /**
+     * On init, spinners are hidden.
+     */
+    test_spinners_default_hidden: function(){
+        var navigator = this.get_navigator();
+        var spinner = navigator.get('spinners').item(0);
+        Y.Assert.areEqual('hidden', spinner.getStyle('visibility'));
+    },
+
+    /**
+     * load_model shows the spinners.
+     */
+    test_load_model_shows_spinner: function(){
+        var navigator = this.get_navigator();
+        var spinner = navigator.get('spinners').item(0);
+        navigator.load_model({});
+        Y.Assert.areEqual('visible', spinner.getStyle('visibility'));
+    },
+
+    /**
+     * load_model ignores the spinners if fetch_only is set.
+     */
+    test_load_model_fetch_only_ignores_spinner: function(){
+        var navigator = this.get_navigator();
+        var spinner = navigator.get('spinners').item(0);
+        navigator.load_model({}, true);
+        Y.Assert.areEqual('hidden', spinner.getStyle('visibility'));
+    },
+
+    /**
+     * After a successful load, the spinners are hidden.
+     */
+    test_success_clears_spinner: function(){
+        var navigator = this.get_navigator();
+        var spinner = navigator.get('spinners').item(0);
+        var mock_io = navigator.get('io_provider');
+        navigator.load_model({});
+        mock_io.last_request.successJSON({mustache_model: {bugtasks: []}});
+        Y.Assert.areEqual('hidden', spinner.getStyle('visibility'));
+    },
+
+    /**
+     * After a successful load with fetch_only, spinners are left alone.
+     */
+    test_fetch_only_success_ignores_spinner: function(){
+        var navigator = this.get_navigator();
+        var spinner = navigator.get('spinners').item(0);
+        var mock_io = navigator.get('io_provider');
+        navigator.set_pending(true);
+        navigator.load_model({}, true);
+        mock_io.last_request.successJSON({mustache_model: {bugtasks: []}});
+        Y.Assert.areEqual('visible', spinner.getStyle('visibility'));
+    },
+
+    /**
+     * When a load fails, the spinners are hidden
+     */
+    test_failure_clears_spinner: function(){
+        var navigator = this.get_navigator();
+        var spinner = navigator.get('spinners').item(0);
+        var mock_io = navigator.get('io_provider');
+        navigator.load_model({});
+        mock_io.failure();
+        Y.Assert.areEqual('hidden', spinner.getStyle('visibility'));
+    },
+
+    /**
+     * When a fetch-only load fails, the spinners are left alone.
+     */
+    test_fetch_only_failure_ignores_spinner: function(){
+        var navigator = this.get_navigator();
+        var spinner = navigator.get('spinners').item(0);
+        var mock_io = navigator.get('io_provider');
+        navigator.load_model({}, true);
+        navigator.set_pending(true);
+        mock_io.failure();
+        Y.Assert.areEqual('visible', spinner.getStyle('visibility'));
+    }
+}));
+
 var handle_complete = function(data) {
     window.status = '::::' + JSON.stringify(data);
     };