launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/float-banners into lp:launchpad.

Commit message:
Make the beta and privacy banners float over the rest of the page when scrolling.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/float-banners/+merge/283537

Make the beta and privacy banners float over the rest of the page when scrolling, so that it's harder to forget that a page is private.  This roughly mirrors the behaviour on {bazaar,git}.launchpad.net, but with extra complications to cope with the fact that there may be more than one banner and that the privacy banner can appear or disappear on the fly.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/float-banners into lp:launchpad.

=== modified file 'lib/lp/app/javascript/ui/assets/skins/sam/banner.css'
--- lib/lp/app/javascript/ui/assets/skins/sam/banner.css	2012-11-12 18:21:12 +0000
+++ lib/lp/app/javascript/ui/assets/skins/sam/banner.css	2016-01-21 19:04:48 +0000
@@ -19,12 +19,12 @@
  * This also needs to be updated
  */
 body.beta #locationbar, body.private #locationbar {
-    top: 47px;
+    padding-top: 47px;
 }
 
 /* If we have both classes make room for two banners height */
 body.beta.private #locationbar {
-    top: 94px;
+    padding-top: 94px;
 }
 
 /* If the container exists make sure we start out with the rest of the page
@@ -41,8 +41,11 @@
     display: block;
     font-size: 14px;
     font-weight: bold;
+    left: 0;
     line-height: 21px;
     padding: 8px 20px;
+    position: fixed;
+    right: 0;
     text-align: left;
     text-shadow: 0 -1px 0 #631616;
     z-index: 10;

=== modified file 'lib/lp/app/javascript/ui/banner.js'
--- lib/lp/app/javascript/ui/banner.js	2014-05-29 15:02:53 +0000
+++ lib/lp/app/javascript/ui/banner.js	2016-01-21 19:04:48 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright 2012 Canonical Ltd.  This software is licensed under the
+ * Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Notification banner widget
@@ -37,6 +37,22 @@
             '</div>'
         ].join(''),
 
+        /**
+         * Position banners and body appropriately.
+         *
+         * @method _set_positions
+         */
+        _set_positions: function () {
+            var banners = Y.all('.yui3-banner-content');
+            banners.each(function (banner, i) {
+                banner.setStyle('top', (i * 47) + 'px');
+            });
+            var location_bar = Y.one('body #locationbar');
+            if (Y.Lang.isValue(location_bar)) {
+                location_bar.setStyle(
+                    'padding-top', (banners.size() * 47) + 'px');
+            }
+        },
 
         /**
          * Bind events that our widget supports such as closing the banner.
@@ -112,6 +128,7 @@
             var _node = this.get('boundingBox')._node;
             var getComputedStyle = document.defaultView.getComputedStyle;
             _node.style.display = getComputedStyle(_node).display;
+            this._set_positions();
             return this.set('visible', true);
         }
 
@@ -304,10 +321,10 @@
 
             /**
              * Manually force the banner type so users don't need to set it.
-             * This is a beta banner class.
+             * This is a private banner class.
              *
              * @attribute banner_type
-             * @default BETA
+             * @default PRIVATE
              * @type {String}
              */
             banner_type: {

=== modified file 'lib/lp/app/javascript/views/global.js'
--- lib/lp/app/javascript/views/global.js	2012-11-09 18:46:31 +0000
+++ lib/lp/app/javascript/views/global.js	2016-01-21 19:04:48 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright 2012 Canonical Ltd.  This software is licensed under the
+ * Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Global View handler for Launchpad
@@ -59,7 +59,8 @@
 
                     // There is no current container for the banner since
                     // we're creating it on the fly.
-                    var container = Y.Node.create('<div>');
+                    var container = Y.Node.create(
+                        '<div class="private_banner_container">');
 
                     // XXX: Bug #1076074
                     var body = Y.one('body');

=== modified file 'lib/lp/app/javascript/views/tests/test_global.html'
--- lib/lp/app/javascript/views/tests/test_global.html	2012-11-09 18:46:31 +0000
+++ lib/lp/app/javascript/views/tests/test_global.html	2016-01-21 19:04:48 +0000
@@ -1,7 +1,7 @@
 <!DOCTYPE html>
 
 <!--
-Copyright 2012 Canonical Ltd.  This software is licensed under the
+Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
 GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -54,6 +54,7 @@
 
     </head>
     <body class="yui3-skin-sam">
+        <div id="locationbar"></div>
         <ul id="suites">
             <li>lp.views.global.test</li>
         </ul>

=== modified file 'lib/lp/app/javascript/views/tests/test_global.js'
--- lib/lp/app/javascript/views/tests/test_global.js	2013-03-20 03:41:40 +0000
+++ lib/lp/app/javascript/views/tests/test_global.js	2016-01-21 19:04:48 +0000
@@ -1,4 +1,4 @@
-/* Copyright 2012 Canonical Ltd.  This software is licensed under the
+/* Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE). */
 
 YUI.add('lp.views.global.test', function (Y) {
@@ -61,6 +61,11 @@
                 '',
                 this.container.get('innerHTML'),
                 'The container is still empty.');
+
+            // Positioning is correct.
+            var location_bar = Y.one('body #locationbar');
+            Y.Assert.areEqual('0px', location_bar.getStyle('padding-top'));
+
             view.destroy();
         },
 
@@ -79,6 +84,12 @@
                 banner_node,
                 'The container has a new banner node in there.');
 
+            // Positioning is correct.
+            var container_node = Y.one('.yui3-banner-content.beta');
+            Y.Assert.areEqual('0px', container_node.getStyle('top'));
+            var location_bar = Y.one('body #locationbar');
+            Y.Assert.areEqual('47px', location_bar.getStyle('padding-top'));
+
             view.destroy();
         },
 
@@ -102,6 +113,14 @@
                 banner_nodes._nodes.length,
                 'We should have two banners rendered.');
 
+            // Positioning is correct.
+            var beta_container_node = Y.one('.yui3-banner-content.beta');
+            Y.Assert.areEqual('0px', beta_container_node.getStyle('top'));
+            var private_container_node = Y.one('.yui3-banner-content.private');
+            Y.Assert.areEqual('47px', private_container_node.getStyle('top'));
+            var location_bar = Y.one('body #locationbar');
+            Y.Assert.areEqual('94px', location_bar.getStyle('padding-top'));
+
             view.destroy();
         },
 
@@ -124,6 +143,12 @@
                 banner_text.indexOf(msg),
                 'The event text is turned into the banner content');
 
+            // Positioning is correct.
+            var container_node = Y.one('.yui3-banner-content.private');
+            Y.Assert.areEqual('0px', container_node.getStyle('top'));
+            var location_bar = Y.one('body #locationbar');
+            Y.Assert.areEqual('47px', location_bar.getStyle('padding-top'));
+
            // Manually clean up.
            Y.one('.yui3-banner').remove(true);
            view.destroy();