Merge lp:~frankban/juju-gui/bug-1090046-resize into lp:juju-gui/experimental

[Francesco Banconi]

Fix GUI breakage on browser resizing.

Fixed how yui events are handled by the component framework.
Also added two tests:
- ensure that the module synthetic events binding works;
- ensure that the custom events 'beforePageSizeRecalculation'
  and 'afterPageSizeRecalculation' are correctly fired by
  the environment view when the browser is resized.

QA: resize the browser, open the charm panel, resize the
    browser again, close the charm panel, resize again,
    re-open the panel...

https://codereview.appspot.com/6941053/

[Francesco Banconi (frankban)]

Reviewers: mp+139959_code.launchpad.net,

Message:
Please take a look.

Description:
Fix GUI breakage on browser resizing.

Fixed how yui events are handled by the component framework.
Also added two tests:
- ensure that the module synthetic events binding works;
- ensure that the custom events 'beforePageSizeRecalculation'
   and 'afterPageSizeRecalculation' are correctly fired by
   the environment view when the browser is resized.

QA: resize the browser, open the charm panel, resize the
     browser again, close the charm panel, resize again,
     re-open the panel...

https://code.launchpad.net/~frankban/juju-gui/bug-1090046-resize/+merge/139959

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M app/assets/javascripts/d3-components.js
   M test/test_d3_components.js
   M test/test_environment_view.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: test/test_d3_components.js
=== modified file 'test/test_d3_components.js'
--- test/test_d3_components.js 2012-11-09 14:17:58 +0000
+++ test/test_d3_components.js 2012-12-14 15:25:55 +0000
@@ -129,6 +129,24 @@

       });

+ it('should correctly handle synthetic event bindings', function(done) {
+ comp = new NS.Component();
+ comp.setAttrs({container: container});
+ modA = new TestModule();
+ var resized = false;
+ modA.windowResizeHandler = function(evt) {
+ resized = true;
+ };
+ modA.events.yui.windowresize = 'windowResizeHandler';
+ comp.addModule(modA);
+ var subscription = Y.after('windowresize', function(evt) {
+ subscription.detach();
+ assert.isTrue(resized);
+ done();
+ });
+ Y.one('window').simulate('resize');
+ });
+
    it('should support basic rendering from all modules',
       function() {
         var modA = new TestModule(),

Index: test/test_environment_view.js
=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js 2012-12-05 18:37:01 +0000
+++ test/test_environment_view.js 2012-12-14 15:25:55 +0000
@@ -103,7 +103,7 @@
      });

      beforeEach(function(done) {
- container = Y.Node.create('<div id="test-container" />');
+ container = Y.Node.create('<div
/>').setStyle('visibility', 'hidden');
        Y.one('body').prepend(container);
        db = new models.Database();
        db.on_delta({data: environment_delta});
@@ -118,6 +118,22 @@
        done();
      });

+ it('must handle the window resize event', function(done) {
+ var view = new views.environment({container: container, db: db});
+ view.render();
+ var beforeResizeEventFired = false;
+ Y.once('beforePageSizeRecalculation', function() {
+ // This event must be fired by
views.MegaModule.setSizesFromViewport.
+ beforeResizeEventFired = true;
+ });
+ Y.once('afterPageSizeRecalculati...

[Benjamin Saller (bcsaller)]

Thanks,

This is a nice catch, a good fix and glad to see the additional tests.

+1 LGTM

https://codereview.appspot.com/6941053/diff/1/app/assets/javascripts/d3-components.js
File app/assets/javascripts/d3-components.js (right):

https://codereview.appspot.com/6941053/diff/1/app/assets/javascripts/d3-components.js#newcode238
app/assets/javascripts/d3-components.js:238: var callback =
Y.bind(handler.callback, handler.context);
This is a nice fix, thank you.

https://codereview.appspot.com/6941053/

[Gary Poster (gary)]

Land as is.

Thanks, Francesco! Great to have the user-facing fix and the general
event handling fix, and the branch has nice tests.

Gary

https://codereview.appspot.com/6941053/

[Francesco Banconi (frankban)]

*** Submitted:

Fix GUI breakage on browser resizing.

Fixed how yui events are handled by the component framework.
Also added two tests:
- ensure that the module synthetic events binding works;
- ensure that the custom events 'beforePageSizeRecalculation'
   and 'afterPageSizeRecalculation' are correctly fired by
   the environment view when the browser is resized.

QA: resize the browser, open the charm panel, resize the
     browser again, close the charm panel, resize again,
     re-open the panel...

R=benjamin.saller, gary.poster
CC=
https://codereview.appspot.com/6941053

https://codereview.appspot.com/6941053/

=== modified file 'app/assets/javascripts/d3-components.js'
--- app/assets/javascripts/d3-components.js	2012-12-11 15:35:25 +0000
+++ app/assets/javascripts/d3-components.js	2012-12-14 16:41:29 +0000
@@ -231,9 +231,12 @@
           // Bind resolved event handlers as a group.
           if (Y.Object.keys(resolvedHandler).length) {
             Y.each(resolvedHandler, function(handler, name) {
-              subscriptions.push(Y[eventPhase](name,
-                                               handler.callback,
-                                               handler.context));
+              // DOM and synthetic events are subscribed using Y.on with
+              // this signature: Y.on(event, callback, target, context).
+              // For this reason, it is not possible here to just pass the
+              // context as third argument.
+              var callback = Y.bind(handler.callback, handler.context);
+              subscriptions.push(Y[eventPhase](name, callback));
             });
           }
         });

=== modified file 'test/test_d3_components.js'
--- test/test_d3_components.js	2012-11-09 14:17:58 +0000
+++ test/test_d3_components.js	2012-12-14 16:41:29 +0000
@@ -129,6 +129,24 @@
 
      });
 
+  it('should correctly handle synthetic event bindings', function(done) {
+    comp = new NS.Component();
+    comp.setAttrs({container: container});
+    modA = new TestModule();
+    var resized = false;
+    modA.windowResizeHandler = function(evt) {
+      resized = true;
+    };
+    modA.events.yui.windowresize = 'windowResizeHandler';
+    comp.addModule(modA);
+    var subscription = Y.after('windowresize', function(evt) {
+      subscription.detach();
+      assert.isTrue(resized);
+      done();
+    });
+    Y.one('window').simulate('resize');
+  });
+
   it('should support basic rendering from all modules',
      function() {
        var modA = new TestModule(),

=== modified file 'test/test_environment_view.js'
--- test/test_environment_view.js	2012-12-05 18:37:01 +0000
+++ test/test_environment_view.js	2012-12-14 16:41:29 +0000
@@ -103,7 +103,7 @@
     });
 
     beforeEach(function(done) {
-      container = Y.Node.create('<div id="test-container" />');
+      container = Y.Node.create('<div />').setStyle('visibility', 'hidden');
       Y.one('body').prepend(container);
       db = new models.Database();
       db.on_delta({data: environment_delta});
@@ -118,6 +118,22 @@
       done();
     });
 
+    it('must handle the window resize event', function(done) {
+      var view = new views.environment({container: container, db: db});
+      view.render();
+      var beforeResizeEventFired = false;
+      Y.once('beforePageSizeRecalculation', function() {
+        // This event must be fired by views.MegaModule.setSizesFromViewport.
+        beforeResizeEventFired = true;
+      });
+      Y.once('afterPageSizeRecalculation', function() {
+        // This event must be fired by views.MegaModule.setSizesFromViewport.
+        assert.isTrue(beforeResizeEventFired);
+        done();
+      });
+      Y.one('window').simulate('resize');
+    });
+
     // Ensure the environment view loads properly
     it('must be able to render service blocks and relations',
         function() {