launchpad-reviewers team mailing list archive

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

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/banner-quick-fix/+merge/105500

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 that is set based on the banner
object name (so 'banner' for the base object, or 'privacyBanner' for the
privacy banner). If it exists, it merely updates the markup, rather than
creating a new set.

The on visibleChange event now checks if the banner is currently hidden,
rather than using the object's visible attribute, as the value of visible
cannot be guaranteed during the change.

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-quick-fix/+merge/105500
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/banner-quick-fix into 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-11 16:24:20 +0000
@@ -7,7 +7,6 @@
  */
 
 YUI.add('lp.app.banner', function (Y) {
-
 var ns = Y.namespace('lp.app.banner');
 
 ns.Banner = Y.Base.create('banner', Y.Widget, [], {
@@ -75,10 +74,11 @@
         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(
             this.get('banner_template'),
@@ -86,18 +86,32 @@
         this.get('target').append(banner_html);
     },
 
+    _updateBanner: function(existing_banner) {
+        existing_banner.set('text', this.get('notification_text'));
+    },
+
+    renderUI: function() {
+        var existing_banner = Y.one('#' + this.get('banner_id'));
+        if (existing_banner) {
+            this._updateBanner(existing_banner); 
+        } else {
+            this._createBanner(); 
+        }
+    },
+
     bindUI: function() {
         this.on('visibleChange', function() {
-            visible = this.get('visible'); 
-            if (visible) {
+            var is_hidden = Y.one('.global-notification').hasClass('hidden');
+            if (is_hidden) {
+                this._showBanner(); 
+            } else {
                 this._hideBanner(); 
-            } else {
-                this._showBanner(); 
             }
         }); 
     }
 }, {
     ATTRS: {
+        banner_id: { value: this.NAME + "-banner-id" },
         notification_text: { value: "" },
         banner_icon: { value: "<span></span>" },
         target: {
@@ -108,14 +122,15 @@
         banner_template: {
             valueFn: function() {
                 return [
-                    '<div class="global-notification transparent hidden">', 
+                    '<div id="{{ banner_id }}"', 
+                        'class="global-notification transparent hidden">', 
                         '{{{ badge }}}',
                         '{{ text }}',
                     "</div>"].join('')
             } 
         },
-        visible: { 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-11 16:24:20 +0000
@@ -1,4 +1,4 @@
-/* Copyright (c) 2012, Canonical Ltd. All rights reserved. */
+/*  Copyright (c) 2012, Canonical Ltd. All rights reserved. */
 
 YUI.add('lp.app.banner.test', function (Y) {
 
@@ -47,8 +47,7 @@
         test_render_no_config: function () {
             var banner = new Y.lp.app.banner.Banner(); 
             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'));
         },
@@ -60,8 +59,7 @@
             };
             var banner = new Y.lp.app.banner.Banner(cfg); 
             banner.render();
-            var main = Y.one("#maincontent");
-            var banner_node = main.one(".global-notification");
+            var banner_node = Y.one(".global-notification");
             var badge = banner_node.get('children').pop();
             Y.Assert.isObject(banner_node);
             Y.Assert.areEqual(cfg.notification_text, banner_node.get('text'));
@@ -71,9 +69,8 @@
         test_show: function() {
             var banner = new Y.lp.app.banner.Banner(); 
             banner.render();
-
-            var main = Y.one("#maincontent");
-            var banner_node = main.one(".global-notification");
+            
+            var banner_node = Y.one(".global-notification");
             var wait_for_hide_animation = 320;
             var check = function () { 
                 Y.Assert.isFalse(banner_node.hasClass('hidden'));
@@ -88,8 +85,7 @@
             banner.render();
             banner.show();
 
-            var main = Y.one("#maincontent");
-            var banner_node = main.one(".global-notification");
+            var banner_node = Y.one(".global-notification");
             var wait_for_hide_animation = 320;
             var check = function () { 
                 Y.Assert.isTrue(banner_node.hasClass('hidden'));
@@ -97,10 +93,25 @@
 
             banner.hide();
             this.wait(check, wait_for_hide_animation);
+        },
+
+        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(); 
+            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
+            }); 
+            banner2.render();
+            banner2.show();
+            Y.Assert.areEqual(1, Y.all('.global-notification').size());
+            Y.Assert.areEqual(
+                new_text,
+                Y.one('.global-notification').get('text'));
         }
-
-
-
     }));
-
 }, '0.1', {'requires': ['test', 'console', 'lp.app.banner']});