Merge lp:~huwshimi/juju-gui/visual-update-2 into lp:juju-gui/experimental

[Huw Wilkins]

Visual update #2

This completes the work from lp:~rharding/juju-gui/new-controls and lp:~huwshimi/juju-gui/visual-update-1 in adding the new header and making it work.

https://codereview.appspot.com/10904043/

[Huw Wilkins (huwshimi)]

Reviewers: mp+172739_code.launchpad.net,

Message:
Please take a look.

Description:
Visual update #2

This completes the work from lp:~rharding/juju-gui/new-controls and
lp:~huwshimi/juju-gui/visual-update-1 in adding the new header and
making it work.

https://code.launchpad.net/~huwshimi/juju-gui/visual-update-2/+merge/172739

Requires:
https://code.launchpad.net/~rharding/juju-gui/new-controls/+merge/172631

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10904043/

Affected files:
   A [revision details]
   M app/subapps/browser/browser.js
   M app/subapps/browser/templates/fullscreen.handlebars
   M app/subapps/browser/templates/minimized.handlebars
   M app/subapps/browser/templates/sidebar.handlebars
   M app/templates/browser-search.handlebars
   M lib/views/browser/browser-icon.less
   M lib/views/browser/bws-searchbox.less
   M lib/views/browser/main.less
   M test/test_browser_search_widget.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: app/templates/browser-search.handlebars
=== modified file 'app/templates/browser-search.handlebars'
--- app/templates/browser-search.handlebars 2013-07-02 17:19:34 +0000
+++ app/templates/browser-search.handlebars 2013-07-03 05:20:36 +0000
@@ -1,2 +1,1 @@
-<a href="" class="bws-icon"></a>
  {{> bws-searchbox }}

Index: app/subapps/browser/browser.js
=== modified file 'app/subapps/browser/browser.js'
--- app/subapps/browser/browser.js 2013-06-27 18:47:57 +0000
+++ app/subapps/browser/browser.js 2013-07-03 05:20:36 +0000
@@ -48,13 +48,16 @@

       */
      _detailsVisible: function(visible) {
- var detailsNode = Y.one('.bws-view-data');
+ var detailsNode = Y.one('.bws-view-data'),
+ container = Y.one('.charmbrowser');
        if (detailsNode) {
          if (visible) {
            detailsNode.show();
+ container.addClass('content-visible');
          }
          else {
            detailsNode.hide();
+ container.removeClass('content-visible');
          }
        }
      },

Index: lib/views/browser/browser-icon.less
=== modified file 'lib/views/browser/browser-icon.less'
--- lib/views/browser/browser-icon.less 2013-07-01 06:16:17 +0000
+++ lib/views/browser/browser-icon.less 2013-07-03 05:20:36 +0000
@@ -1,7 +1,7 @@
  .bws-icon {
      position: absolute;
      display: block;
- right: -38px;
+ left: 25%;
      top: 110px;
      width: 26px;
      height: 26px;
@@ -12,3 +12,6 @@
      border-top-right-radius: 4px;
      border-bottom-right-radius: 4px;
  }
+.content-visible .bws-icon {
+ left: 75%;
+}

Index: app/subapps/browser/templates/fullscreen.handlebars
=== modified file 'app/subapps/browser/templates/fullscreen.handlebars'
--- app/subapps/browser/templates/fullscreen.handlebars 2013-04-03 01:48:42
+0000
+++ app/subapps/browser/templates/fullscreen.handlebars 2013-07-03 05:30:55
+0000
@@ -1,4 +1,5 @@
 ...

[Richard Harding (rharding)]

I don't follow this change. It adds a regression where the show/hide
icon doesn't move. It appears the change is centered around moving the
bws-icon to other templates and trying to add the concept of
content-visible css class. The issue is that you're hooking this class
up into if the details of a charm is visible which can be true/false
which the sidebar or fullscreen (.charmbrowser) is always there. I'm not
following why they've been associated.

https://codereview.appspot.com/10904043/

[lp:~huwshimi/juju-gui/visual-update-2]

795. By Huw Wilkins on 2013-07-03

Merged with trunk.

796. By Huw Wilkins on 2013-07-03

Fixed browser icon positioning when at small screen sizes. Made icon orange when browser is minimised.

[Huw Wilkins (huwshimi)]

On 2013/07/03 13:19:39, rharding wrote:
> I don't follow this change. It adds a regression where the show/hide
icon
> doesn't move. It appears the change is centered around moving the
bws-icon to
> other templates and trying to add the concept of content-visible css
class. The
> issue is that you're hooking this class up into if the details of a
charm is
> visible which can be true/false which the sidebar or fullscreen
(.charmbrowser)
> is always there. I'm not following why they've been associated.

The positioning bug was caused by the code for small screen sizes not
accounting for the minimised browser. I've pushed a change for that and
additionally makes the icon orange when the sidebar is minimised.

https://codereview.appspot.com/10904043/

[lp:~huwshimi/juju-gui/visual-update-2]

797. By Huw Wilkins on 2013-07-03

Fixed top position of configure panel.

[Huw Wilkins (huwshimi)]

Please take a look.

https://codereview.appspot.com/10904043/

[Gary Poster (gary)]

LGTM. Thank you!

The diff is crazy on rietveld but looks good locally.

https://codereview.appspot.com/10904043/

[Huw Wilkins (huwshimi)]

*** Submitted:

Visual update #2

This completes the work from lp:~rharding/juju-gui/new-controls and
lp:~huwshimi/juju-gui/visual-update-1 in adding the new header and
making it work.

R=rharding, gary.poster
CC=
https://codereview.appspot.com/10904043

https://codereview.appspot.com/10904043/

=== modified file 'app/subapps/browser/browser.js'
--- app/subapps/browser/browser.js	2013-07-03 21:38:35 +0000
+++ app/subapps/browser/browser.js	2013-07-03 23:07:27 +0000
@@ -48,13 +48,16 @@
 
      */
     _detailsVisible: function(visible) {
-      var detailsNode = Y.one('.bws-view-data');
+      var detailsNode = Y.one('.bws-view-data'),
+          container = Y.one('.charmbrowser');
       if (detailsNode) {
         if (visible) {
           detailsNode.show();
+          container.addClass('content-visible');
         }
         else {
           detailsNode.hide();
+          container.removeClass('content-visible');
         }
       }
     },

=== modified file 'app/subapps/browser/templates/fullscreen.handlebars'
--- app/subapps/browser/templates/fullscreen.handlebars	2013-04-03 01:48:42 +0000
+++ app/subapps/browser/templates/fullscreen.handlebars	2013-07-03 23:07:27 +0000
@@ -1,4 +1,5 @@
 <div class="charmbrowser">
+    <a href="" class="bws-icon"></a>
     <div id="bws-fullscreen">
         <div class="bws-header">
         </div>

=== modified file 'app/subapps/browser/templates/minimized.handlebars'
--- app/subapps/browser/templates/minimized.handlebars	2013-07-03 17:35:21 +0000
+++ app/subapps/browser/templates/minimized.handlebars	2013-07-03 23:07:27 +0000
@@ -1,12 +1,1 @@
 <a href="" class="bws-icon"></a>
-{{! This should include the bws-searchbox partial template, but until we have
-    the correct context it will error. }}
-<div class="bws-searchbox">
-    <form>
-        <img src="/juju-ui/assets/images/browser_searchbox_search_icon.png"
-            alt="Search" class="search" />
-        <input type="search" name="bws-search"
-            value=""
-            placeholder="Search for Charms"/>
-    </form>
-</div>

=== modified file 'app/subapps/browser/templates/sidebar.handlebars'
--- app/subapps/browser/templates/sidebar.handlebars	2013-06-14 18:05:08 +0000
+++ app/subapps/browser/templates/sidebar.handlebars	2013-07-03 23:07:27 +0000
@@ -1,4 +1,5 @@
 <div class="charmbrowser">
+    <a href="" class="bws-icon"></a>
     <div id="bws-sidebar">
         <div class="bws-header">
         </div>

=== modified file 'app/templates/browser-search.handlebars'
--- app/templates/browser-search.handlebars	2013-07-03 17:35:21 +0000
+++ app/templates/browser-search.handlebars	2013-07-03 23:07:27 +0000
@@ -1,2 +1,1 @@
-<a href="" class="bws-icon"></a>
 {{> bws-searchbox }}

=== modified file 'lib/views/browser/browser-icon.less'
--- lib/views/browser/browser-icon.less	2013-07-03 17:35:21 +0000
+++ lib/views/browser/browser-icon.less	2013-07-03 23:07:27 +0000
@@ -1,7 +1,7 @@
 .bws-icon {
     position: absolute;
     display: block;
-    right: -38px;
+    left: 25%;
     top: 110px;
     width: 26px;
     height: 26px;
@@ -12,3 +12,6 @@
     border-top-right-radius: 4px;
     border-bottom-right-radius: 4px;
 }
+.content-visible .bws-icon {
+    left: 75%;
+}

=== modified file 'lib/views/browser/main.less'
--- lib/views/browser/main.less	2013-07-03 17:35:21 +0000
+++ lib/views/browser/main.less	2013-07-03 23:07:27 +0000
@@ -179,4 +179,13 @@
     left: @bws-sidebar-min-width;
     min-width: 700px;
   }
+  .bws-icon {
+    left: @bws-sidebar-min-width !important;
+  }
+  .content-visible .bws-icon {
+    left: @bws-sidebar-min-width + 700 !important;
+  }
+  #subapp-browser-min .bws-icon {
+    left: 0 !important;
+  }
 }

=== modified file 'lib/views/browser/minimized.less'
--- lib/views/browser/minimized.less	2013-07-03 17:35:21 +0000
+++ lib/views/browser/minimized.less	2013-07-03 23:07:27 +0000
@@ -9,5 +9,6 @@
     .bws-icon {
         left: 0;
         right: auto;
+        background-color: @charm-panel-orange;
     }
 }

=== modified file 'lib/views/stylesheet.less'
--- lib/views/stylesheet.less	2013-07-03 17:35:21 +0000
+++ lib/views/stylesheet.less	2013-07-03 23:07:27 +0000
@@ -1114,7 +1114,7 @@
     display: block;
     z-index: 999;
     padding: 0;
-    top: 70px;
+    top: 61px;
     right: 0;
     position: absolute;
     width: @charm-panel-width;

=== modified file 'test/test_browser_search_widget.js'
--- test/test_browser_search_widget.js	2013-07-03 17:35:21 +0000
+++ test/test_browser_search_widget.js	2013-07-03 23:07:27 +0000
@@ -45,7 +45,6 @@
     var search = new Search();
     search.render(container);
     assert.isObject(container.one('.bws-searchbox'));
-    assert.isObject(container.one('.bws-icon'));
   });
 
   it('should support setting search string', function() {