launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/banner-fixes into lp:launchpad.

Requested reviews:
  Richard Harding (rharding): code

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/banner-fixes/+merge/105729

Summary
=======
The new banner object has a couple of oddities exposed when trying to use it
with the privacy banner. 

Specifically, it's visible behavior is a little unreliable, and certain code
paths can result in multiple banner objects being called, which end up with
multiple renders.

Implementation
==============
The render method now checks to see if there is existing markup for a given
type of banner, by checking for a banner id attribute. If it exists, it merely 
updates the markup, rather than creating a new set.

The banner specific code is now fired `after` rather than `on` the `visibleChange`.

Additionally, per a suggestion from a previous review, a skip_animation attribute has
been added which sets anim times to zero.

Tests
=====
bin/test -vvct test_banner --layer=YUI

QA
==
Again, this branch doesn't wire the banner object into anything--there is
nothing to be QAed.

LoC
===
The total arc of work for this banner code removes the duplication in privacy
and beta notifications, resulting in an LoC reduction.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/banners/tests/test_banner.js
  lib/lp/app/javascript/banners/banner.js
-- 
https://code.launchpad.net/~jcsackett/launchpad/banner-fixes/+merge/105729
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/app/javascript/banners/banner.js'
--- lib/lp/app/javascript/banners/banner.js	2012-05-09 14:51:35 +0000
+++ lib/lp/app/javascript/banners/banner.js	2012-05-14 22:13:22 +0000
@@ -7,11 +7,26 @@
  */
 
 YUI.add('lp.app.banner', function (Y) {
-
 var ns = Y.namespace('lp.app.banner');
 
 ns.Banner = Y.Base.create('banner', Y.Widget, [], {
 
+    _getAnimTimes: function() {
+        if (this.get('skip_animation')) {
+            var anim_times = {
+                fade: 0.0,
+                slide_out: 0.0,
+            }    
+        } else {
+            var anim_times = {
+                fade: 0.3,
+                slide_out: 0.2,
+            }    
+        }
+
+        return anim_times;
+    },
+
     _showBanner: function () {
         var body = Y.one('body');
         body.addClass('global-notification-visible');
@@ -19,21 +34,23 @@
         var global_notification = Y.one('.global-notification');
         global_notification.removeClass('hidden');
 
+        var anim_times = this._getAnimTimes();
+
         var fade_in = new Y.Anim({
             node: global_notification,
             to: {opacity: 1},
-            duration: 0.3
+            duration: anim_times.fade 
         });
         var body_space = new Y.Anim({
             node: body,
             to: {'paddingTop': '40px'},
-            duration: 0.2,
+            duration: anim_times.slide_out,
             easing: Y.Easing.easeOut
         });
         var login_space = new Y.Anim({
             node: '.login-logout',
             to: {'top': '45px'},
-            duration: 0.2,
+            duration: anim_times.slide_out,
             easing: Y.Easing.easeOut
         });
 
@@ -45,22 +62,25 @@
     _hideBanner: function () {
         var body = Y.one('body');
         var global_notification = Y.one('.global-notification');
+        global_notification.addClass('transparent');
+
+        var anim_times = this._getAnimTimes();
 
         var fade_out = new Y.Anim({
             node: global_notification,
             to: {opacity: 0},
-            duration: 0.3
+            duration: anim_times.fade 
         });
         var body_space = new Y.Anim({
             node: body,
             to: {'paddingTop': 0},
-            duration: 0.2,
+            duration: anim_times.slide_out,
             easing: Y.Easing.easeOut
         });
         var login_space = new Y.Anim({
             node: '.login-logout',
             to: {'top': '6px'},
-            duration: 0.2,
+            duration: anim_times.slide_out,
             easing: Y.Easing.easeOut
         });
         fade_out.on('end', function() {
@@ -75,29 +95,44 @@
         login_space.run();
     },
 
-    renderUI: function() {
+    _createBanner: function () {
         var banner_data = {
             badge: this.get('banner_icon'),
-            text: this.get('notification_text')
+            text: this.get('notification_text'),
+            banner_id: this.get('banner_id')
         };
-        var banner_html = Y.lp.mustache.to_html(
+        return Y.lp.mustache.to_html(
             this.get('banner_template'),
             banner_data);
-        this.get('target').append(banner_html);
+    },
+
+    _updateBanner: function (banner_node) {
+        text_node = banner_node.one('.banner-text');
+        text_node.set('innerText', this.get('notification_text'));
+    },
+
+    renderUI: function() {
+        var existing_banner = Y.one('#' + this.get('banner_id'));
+        if (existing_banner) {
+            this._updateBanner(existing_banner);
+        } else {
+            var banner_html = this._createBanner(); 
+            this.get('target').append(banner_html);
+        }
     },
 
     bindUI: function() {
-        this.on('visibleChange', function() {
-            visible = this.get('visible'); 
-            if (visible) {
+        this.after('visibleChange', function() {
+            if (this.get('visible')) {
+                this._showBanner(); 
+            } else {
                 this._hideBanner(); 
-            } else {
-                this._showBanner(); 
             }
         }); 
     }
 }, {
     ATTRS: {
+        banner_id: { value: "base-banner" },
         notification_text: { value: "" },
         banner_icon: { value: "<span></span>" },
         target: {
@@ -108,14 +143,16 @@
         banner_template: {
             valueFn: function() {
                 return [
-                    '<div class="global-notification transparent hidden">', 
+                    '<div id="{{ banner_id }}"', 
+                        'class="global-notification transparent hidden">', 
                         '{{{ badge }}}',
-                        '{{ text }}',
+                        '<span class="banner-text">{{ text }}</span>',
                     "</div>"].join('')
             } 
         },
-        visible: { value: false },
-    },
+        skip_animation: { value: false },
+        visible: { value: false }
+    }
 });
 
 }, '0.1', {'requires': ['base', 'node', 'anim', 'widget', 'lp.mustache']});

=== modified file 'lib/lp/app/javascript/banners/tests/test_banner.js'
--- lib/lp/app/javascript/banners/tests/test_banner.js	2012-05-08 21:32:33 +0000
+++ lib/lp/app/javascript/banners/tests/test_banner.js	2012-05-14 22:13:22 +0000
@@ -27,7 +27,7 @@
         },
 
         test_init_without_config: function () {
-            var banner = new Y.lp.app.banner.Banner(); 
+            var banner = new Y.lp.app.banner.Banner();
             Y.Assert.areEqual("", banner.get('notification_text'));
             Y.Assert.areEqual("<span></span>", banner.get('banner_icon'));
         },
@@ -37,7 +37,7 @@
                 notification_text: "Some text.",
                 banner_icon: '<span class="sprite"></span>'
             };
-            var banner = new Y.lp.app.banner.Banner(cfg); 
+            var banner = new Y.lp.app.banner.Banner(cfg);
             Y.Assert.areEqual(
                 cfg.notification_text,
                 banner.get('notification_text'));
@@ -45,62 +45,75 @@
         },
 
         test_render_no_config: function () {
-            var banner = new Y.lp.app.banner.Banner(); 
+            var banner = new Y.lp.app.banner.Banner({ skip_animation: true });
             banner.render();
-            var main = Y.one("#maincontent");
-            var banner_node = main.one(".global-notification")
+
+            var banner_node = Y.one(".global-notification")
             Y.Assert.isObject(banner_node);
             Y.Assert.isTrue(banner_node.hasClass('hidden'));
         },
-        
+
         test_render_with_config: function () {
             var cfg = {
                 notification_text: "Some text.",
                 banner_icon: '<span class="sprite"></span>',
+                skip_animation: true
             };
-            var banner = new Y.lp.app.banner.Banner(cfg); 
+            var banner = new Y.lp.app.banner.Banner(cfg);
             banner.render();
-            var main = Y.one("#maincontent");
-            var banner_node = main.one(".global-notification");
-            var badge = banner_node.get('children').pop();
+
+            var banner_node = Y.one(".global-notification");
+            console.log(banner_node);
+            var badge = banner_node.one('.sprite');
             Y.Assert.isObject(banner_node);
             Y.Assert.areEqual(cfg.notification_text, banner_node.get('text'));
-            Y.Assert.isTrue(badge.hasClass('sprite'));
+            Y.Assert.isObject(badge);
         },
 
         test_show: function() {
-            var banner = new Y.lp.app.banner.Banner(); 
+            var banner = new Y.lp.app.banner.Banner({ skip_animation: true });
             banner.render();
-
-            var main = Y.one("#maincontent");
-            var banner_node = main.one(".global-notification");
-            var wait_for_hide_animation = 320;
-            var check = function () { 
-                Y.Assert.isFalse(banner_node.hasClass('hidden'));
-            }
-
             banner.show();
-            this.wait(check, wait_for_hide_animation);
+
+            var banner_node = Y.one(".global-notification");
+            Y.Assert.isFalse(banner_node.hasClass('hidden'));
         },
 
         test_hide: function() {
-            var banner = new Y.lp.app.banner.Banner(); 
+            var banner = new Y.lp.app.banner.Banner({ skip_animation: true });
             banner.render();
             banner.show();
 
-            var main = Y.one("#maincontent");
-            var banner_node = main.one(".global-notification");
-            var wait_for_hide_animation = 320;
-            var check = function () { 
+            // Even with animation times set to 0, this test needs a slight
+            // delay in order for the animation end events to fire.
+            var banner_node = Y.one(".global-notification");
+            var wait_for_anim = 20;
+            var check = function () {
                 Y.Assert.isTrue(banner_node.hasClass('hidden'));
             }
-
             banner.hide();
-            this.wait(check, wait_for_hide_animation);
+            this.wait(check, wait_for_anim);
+        },
+
+        test_only_one_banner: function () {
+            // There can only be one banner node for a given type of banner.
+            var banner = new Y.lp.app.banner.Banner({ skip_animation: true });
+            banner.render();
+            banner.show();
+            Y.Assert.areEqual(1, Y.all('.global-notification').size());
+
+            var new_text = 'This is new text';
+            var banner2 = new Y.lp.app.banner.Banner({
+                notification_text: new_text,
+                skip_animation: true
+            });
+            banner2.render();
+            banner2.show();
+            Y.Assert.areEqual(1, Y.all('.global-notification').size());
+            var banner_node = Y.one('.global-notification');
+            Y.Assert.areEqual(
+                new_text,
+                Y.one('.global-notification').get('text'));
         }
-
-
-
     }));
-
 }, '0.1', {'requires': ['test', 'console', 'lp.app.banner']});