launchpad-reviewers team mailing list archive

[Huw Wilkins <huw.wilkins@xxxxxxxxxxxxx>]

Huw Wilkins has proposed merging lp:~huwshimi/maas/morphing-tests into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~huwshimi/maas/morphing-tests/+merge/102056

This branch rewrites a bunch of the morphing widget to make it more testable. It moves the animation creations into their own functions.

It still feels like the test is a horrible nested Jenga tower, but the animations need to be created after the first animation has been run.

Even though the test pulls apart the morph and runs each of the functions separately the main morph() is still run and confirmed working later in the test.

Any suggestion on how to make this cleaner very welcome.
-- 
https://code.launchpad.net/~huwshimi/maas/morphing-tests/+merge/102056
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~huwshimi/maas/morphing-tests into lp:maas.

=== modified file 'src/maasserver/static/js/morph.js'
--- src/maasserver/static/js/morph.js	2012-04-04 04:31:01 +0000
+++ src/maasserver/static/js/morph.js	2012-04-16 07:06:18 +0000
@@ -18,6 +18,10 @@
 
 Morph.NAME = 'morph';
 
+Morph._fade_out;
+Morph._fade_in;
+Morph._resize;
+
 Morph.ATTRS = {
     /**
      * The DOM node to be morphed from.
@@ -43,56 +47,94 @@
         }
     },
 
+    /**
+     * Animate between the original and new content.
+     *
+     * @method morph
+     * @param {Boolean} reverse: whether or not the widget should morph in the
+            new content or return to the original content.
+     */
     morph: function(reverse) {
-        if (reverse){
-            var srcNode = this.get('targetNode');
-            var targetNode = this.get('srcNode');
-        }
-        else {
-            var srcNode = this.get('srcNode');
-            var targetNode = this.get('targetNode');
-        }
+        this._get_nodes(reverse);
         if (this._animate) {
-            var target_height = targetNode.getComputedStyle('height');
-            var fade_out = new Y.Anim({
-                node: targetNode,
-                to: {opacity: 0},
-                duration: 0.2,
-                easing: 'easeOut'
-                });
             var self = this;
-            fade_out.on('end', function () {
-                targetNode.addClass('hidden');
-                srcNode.setStyle('opacity', 0);
-                srcNode.removeClass('hidden');
-                src_height = srcNode.getComputedStyle('height')
-                    .replace('px', '');
-                srcNode.setStyle('height', target_height);
-                var fade_in = new Y.Anim({
-                    node: srcNode,
-                    to: {opacity: 1},
-                    duration: 1,
-                    easing: 'easeIn'
-                    });
-                var resize = new Y.Anim({
-                    node: srcNode,
-                    to: {height: src_height},
-                    duration: 0.5,
-                    easing: 'easeOut'
-                    });
-                resize.on('end', function () {
-                    srcNode.setStyle('height', 'auto');
-                    self.fire('morphed');
-                });
-                fade_in.run();
-                resize.run();
-            });
-            fade_out.run();
-        }
-        else {
-            targetNode.addClass('hidden');
-            srcNode.removeClass('hidden');
-        }
+            this._create_morph_in();
+            this._fade_out.on('end', function () {
+                self._create_morph_in();
+                self._fade_in.run();
+                self._resize.run();
+            });
+            this._fade_out.run();
+        }
+        else {
+            this.target_node.addClass('hidden');
+            this.src_node.removeClass('hidden');
+            this.fire('morphed');
+        }
+    },
+
+    /**
+     * Get the HTML nodes to morph between.
+     *
+     * @method _get_nodes
+     * @param {Boolean} reverse: whether or not the returned nodes should morph
+            in the new content or return to the original content.
+     */
+    _get_nodes: function(reverse) {
+        if (reverse) {
+            this.src_node = this.get('targetNode');
+            this.target_node = this.get('srcNode');
+        }
+        else {
+            this.src_node = this.get('srcNode');
+            this.target_node = this.get('targetNode');
+        }
+    },
+
+    /**
+     * Create the animation for morphing out the original content.
+     *
+     * @method _create_morph_out
+     */
+    _create_morph_out: function() {
+        this.target_height = this.target_node.getComputedStyle('height');
+        this._fade_out = new Y.Anim({
+            node: this.target_node,
+            to: {opacity: 0},
+            duration: 0.2,
+            easing: 'easeOut'
+            });
+    },
+
+    /**
+     * Create the animation for morphing in the new content.
+     *
+     * @method _create_morph_in
+     */
+    _create_morph_in: function() {
+        var self = this;
+        this.target_node.addClass('hidden');
+        this.src_node.setStyle('opacity', 0);
+        this.src_node.removeClass('hidden');
+        var src_height = this.src_node.getComputedStyle('height')
+            .replace('px', '');
+        this.src_node.setStyle('height', this.target_height);
+        this._fade_in = new Y.Anim({
+            node: this.src_node,
+            to: {opacity: 1},
+            duration: 1,
+            easing: 'easeIn'
+            });
+        this._resize = new Y.Anim({
+            node: this.src_node,
+            to: {height: src_height},
+            duration: 0.5,
+            easing: 'easeOut'
+            });
+        this._resize.on('end', function () {
+            self.src_node.setStyle('height', 'auto');
+            self.fire('morphed');
+        });
     }
 });
 

=== modified file 'src/maasserver/static/js/tests/test_morph.js'
--- src/maasserver/static/js/tests/test_morph.js	2012-03-29 04:21:34 +0000
+++ src/maasserver/static/js/tests/test_morph.js	2012-04-16 07:06:18 +0000
@@ -17,8 +17,9 @@
         var cfg = {
             srcNode: '#panel-two',
             targetNode: '#panel-one'
-        }
+        };
         morpher = new module.Morph(cfg);
+        morpher._get_nodes();
         Y.Assert.isFalse(
             Y.one('#panel-one').hasClass('hidden'),
             'The target panel should initially be visible');
@@ -26,12 +27,40 @@
             Y.one('#panel-two').hasClass('hidden'),
             'The source panel should initially be hidden');
         var morphed_fired = false;
+        var fade_in_completed = false;
+        var fade_out_completed = false;
+        var resize_completed = false;
+        morpher._create_morph_out();
         morpher.on('morphed', function() {
             morphed_fired = true;
         });
-        morpher.morph();
+        morpher._fade_out.on('end', function() {
+            fade_out_completed = true;
+            /* The morph in animation can't be created until after the morph
+               out animation has run.
+            */
+            morpher._create_morph_in();
+            morpher._fade_in.on('end', function() {
+                fade_in_completed = true;
+            });
+            morpher._resize.on('end', function() {
+                resize_completed = true;
+            });
+            morpher._fade_in.run();
+            morpher._resize.run();
+        });
+        morpher._fade_out.run();
         this.wait(function() {
             Y.Assert.isTrue(
+                fade_out_completed,
+                'The fade out animation should have completed');
+            Y.Assert.isTrue(
+                fade_in_completed,
+                'The fade in animation should have completed');
+            Y.Assert.isTrue(
+                resize_completed,
+                'The resize animation should have completed');
+            Y.Assert.isTrue(
                 Y.one(cfg.targetNode).hasClass('hidden'),
                 'The target panel should now be hidden');
             Y.Assert.isFalse(