launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04187
[Merge] lp:~danilo/launchpad/expander-anim into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/expander-anim into lp:launchpad with lp:~danilo/launchpad/expand-diffs as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~danilo/launchpad/expander-anim/+merge/67161
= Expander animations =
This branch provides nice, reversible expander animations. With lazr-js moved inside the tree, I add a few notes about the problems I've seen with existing slide-out, slide-in animations, and instead provide a new reversible slide out animation that is smart about pausing and restarting where appropriate.
Since I am actually refactoring the expanders, I've decided to not fix slide-out/slide-in because that would expand the already big scope. Ideally, they would be replaced with the reversible_slide_out animation altogether. Tests are also missing for it: hopefully not a big deal (I do want to provide them for the reversible stuff).
To actually play with it, try lp:~danilo/launchpad/replace-expanders-1.
== Tests ==
lib/lp/app/javascript/tests/test_expander.html
== Demo and Q/A ==
We can only demo and/or QA this when the follow-up branch lands. Not used anywhere yet.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/expander.js
lib/lp/app/javascript/lazr/effects/effects.js
lib/lp/app/javascript/tests/test_expander.js
--
https://code.launchpad.net/~danilo/launchpad/expander-anim/+merge/67161
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/expander-anim into lp:launchpad.
=== modified file 'lib/lp/app/javascript/expander.js'
--- lib/lp/app/javascript/expander.js 2011-07-01 11:37:12 +0000
+++ lib/lp/app/javascript/expander.js 2011-07-07 10:44:25 +0000
@@ -66,6 +66,8 @@
this.config = {};
}
this.loaded = !Y.Lang.isValue(this.config.loader);
+ this._animation = Y.lazr.effects.reversible_slide_out(
+ this.content_node);
// Is setup complete? Skip any animations until it is.
this.fully_set_up = false;
@@ -126,9 +128,36 @@
* Hide or reveal the content node (by adding the "unseen" class to it).
*
* @param expand Are we expanding? If not, we must be collapsing.
+ * @param no_animation {Boolean} Whether to short-circuit the animation?
*/
- foldContentNode: function(expand) {
- this.setContentClassIf(!expand, this.css_classes.unseen);
+ foldContentNode: function(expand, no_animation) {
+ var expander = this;
+ var has_paused = false;
+ if (no_animation === true) {
+ expander.setContentClassIf(
+ !expand, expander.css_classes.unseen);
+ } else {
+ this._animation.set('reverse', !expand);
+
+ if (expand) {
+ // Show when expanding.
+ expander.setContentClassIf(
+ false, expander.css_classes.unseen);
+ } else {
+ // Hide when collapsing but only after
+ // animation is complete.
+ this._animation.once('end', function() {
+ // Only hide if the direction has not been
+ // changed in the meantime.
+ if (this.get('reverse')) {
+ expander.setContentClassIf(
+ true, expander.css_classes.unseen);
+ }
+ });
+ }
+
+ expander._animation.run();
+ }
},
revealIcon: function() {
@@ -154,10 +183,21 @@
*
* @param output A Node or NodeList to replace the contents of the content
* tag with.
+ * @param failed Whether loading has failed and should be retried.
*/
- receive: function(output) {
- // We'll animate this later (if this.fully_set_up is false).
+ receive: function(output, failed) {
+ if (failed === true) {
+ this.loaded = false;
+ }
+ var from_height = this.content_node.getStyle('height');
this.content_node.setContent(output);
+ var to_height = this.content_node.get('scrollHeight');
+ if (this._animation.get('running')) {
+ this._animation.stop();
+ }
+ this._animation.set('to', { height: to_height });
+ this._animation.set('from', { height: from_height });
+ this._animation.run();
},
/*
@@ -175,8 +215,8 @@
* @param expanded Whether the expander is to be rendered in its expanded
* state. If not, it must be in the collapsed state.
*/
- render: function(expanded) {
- this.foldContentNode(expanded);
+ render: function(expanded, no_animation) {
+ this.foldContentNode(expanded, no_animation);
this.setIcon(expanded);
if (expanded && !this.loaded) {
this.load();
@@ -191,11 +231,11 @@
setUp: function() {
var expander = this;
function click_handler(e) {
- e.preventDefault();
+ e.halt();
expander.render(!expander.isExpanded());
}
- this.render(this.isExpanded());
+ this.render(this.isExpanded(), true);
this.icon_node.on('click', click_handler);
this.revealIcon();
this.fully_set_up = true;
@@ -230,4 +270,4 @@
}
namespace.createByCSS = createByCSS;
-}, "0.1", {"requires": ["node"]});
+}, "0.1", {"requires": ["node", "lazr.effects"]});
=== modified file 'lib/lp/app/javascript/lazr/effects/effects.js'
--- lib/lp/app/javascript/lazr/effects/effects.js 2011-06-29 14:56:15 +0000
+++ lib/lp/app/javascript/lazr/effects/effects.js 2011-07-07 10:44:25 +0000
@@ -37,6 +37,121 @@
/**
+ * Gets the desired total height for a node.
+ */
+function default_to_height(node) {
+ return node.get('scrollHeight');
+}
+
+/**
+ * Produces a reversible slide-out animation as an Y.Anim object.
+ *
+ * Simply changing the 'reverse' attribute will pause the animation
+ * and prepare it for restarting with run() from the point it was on.
+ *
+ * The target node obtains the 'lazr-opened' CSS class when open,
+ * 'lazr-closed' when closed.
+ *
+ * @method reversible_slide_out
+ * @public
+ * @param node {Node|HTMLElement|Selector} The node to apply the effect to.
+ * @param user_cfg {Y.Anim config} Additional Y.Anim config parameters.
+ * These will override the default parameters of the same name.
+ * @return {Y.Anim} A new animation instance.
+ */
+namespace.reversible_slide_out = function(node, user_cfg) {
+ var cfg = Y.merge(namespace.slide_effect_defaults, user_cfg);
+ cfg.node = node;
+
+ var node = Y.one(node);
+ if (node === null) {
+ Y.fail("A valid node, HTMLElement, or CSS3 selector must be given " +
+ "for the slide_out animation.");
+ return null;
+ }
+
+ // We don't want to stomp on what the user may have given as the
+ // from.height and to.height;
+ cfg.from = cfg.from ? cfg.from : {};
+ cfg.from.height = cfg.from.height ? cfg.from.height : 0;
+
+ cfg.to = cfg.to ? cfg.to : {};
+ cfg.to.height = cfg.to.height ? cfg.to.height : default_to_height;
+
+ var anim = new Y.Anim(cfg);
+ node.addClass(OPENED);
+
+ // Set what we need to calculate the new content's scrollHeight.
+ // The call-site must ensure we can find the height (iow, display
+ // CSS class should be set properly; setting 'display: block' will
+ // mess up rendering of table rows when used on them).
+ node.setStyles({
+ height: cfg.from.height,
+ overflow: 'hidden'
+ });
+
+ anim.after('reverseChange', function() {
+ if (this.get('running')) {
+ // Reversing an animation direction always stops it if running.
+ this.stop();
+
+ // Store the current height as appropriate for the direction.
+ var current_height = node.getStyle('height');
+ var full_height = node.get('scrollHeight');
+
+ if (this.get('reverse')) {
+ // Animate from current point to closing.
+ this.set('from', { height: '0px' });
+ this.set('to', { height: current_height });
+ } else {
+ // Animate from current point to the fully open node.
+ this.set('from', { height: current_height });
+ this.set('to', { height: full_height });
+ }
+ } else {
+ // Restore the default from/to if we are not in the middle
+ // of the animation.
+ this.set('from', { height: '0px' });
+ this.set('to', { height: default_to_height });
+ }
+
+ });
+
+ anim.on('end', function() {
+ if (this.get('reverse')) {
+ node.addClass(OPENED).removeClass(CLOSED);
+ } else {
+ node.addClass(CLOSED).removeClass(OPENED);
+ }
+ });
+ return anim;
+
+};
+
+
+/**
+ * Produces a reversible slide-in animation as an Y.Anim object.
+ *
+ * Returned anim object is a reversed slide-out animation.
+ *
+ * The target node obtains the 'lazr-opened' CSS class when open,
+ * 'lazr-closed' when closed.
+ *
+ * @method reversible_slide_in
+ * @public
+ * @param node {Node|HTMLElement|Selector} The node to apply the effect to.
+ * @param user_cfg {Y.Anim config} Additional Y.Anim config parameters.
+ * These will override the default parameters of the same name.
+ * @return {Y.Anim} A new animation instance.
+ */
+namespace.reversible_slide_in = function(node, user_cfg) {
+ var anim = namespace.reversible_slide_out(node, user_cfg);
+ anim.set('reverse', true);
+ return anim;
+}
+
+
+/**
* Produces a simple slide-out drawer effect as a Y.Anim object.
*
* Starts by setting the container's overflow to 'hidden', display to 'block',
@@ -48,6 +163,9 @@
* 'lazr-closed' when closed.
*
* This animation is reversible.
+ * XXX 20110704 Danilo:
+ * Reversing doesn't actually make the animation restart from
+ * where it was stopped, so "jerking" effect can still be seen.
*
* @method slide_out
* @public
@@ -70,10 +188,6 @@
return null;
}
- var default_to_height = function(node) {
- return node.get('scrollHeight');
- };
-
// We don't want to stomp on what the user may have given as the
// from.height and to.height;
cfg.from = cfg.from ? cfg.from : {};
@@ -163,6 +277,8 @@
anim.on('start', function() {
if (!this.drawer_closed) {
// We're closing the draw, so hide the overflow.
+ // XXX 20110704 Danilo: this logic seems broken for reversed
+ // animations.
node.setStyle('overflow', 'hidden');
}
});
@@ -171,6 +287,8 @@
if (this.drawer_closed) {
// We've finished opening the drawer, so show the overflow, just
// to be safe.
+ // XXX 20110704 Danilo: this logic seems broken for reversed
+ // animations.
this.drawer_closed = false;
node.setStyle('overflow', 'visible')
.addClass(OPENED)
=== modified file 'lib/lp/app/javascript/tests/test_expander.js'
--- lib/lp/app/javascript/tests/test_expander.js 2011-07-01 11:37:12 +0000
+++ lib/lp/app/javascript/tests/test_expander.js 2011-07-07 10:44:25 +0000
@@ -6,8 +6,32 @@
base: '../../../../canonical/launchpad/icing/yui/',
filter: 'raw', combine: false,
fetchCSS: false
- }).use('test', 'console', 'node', 'node-event-simulate',
- 'lp.app.widgets.expander', function(Y) {
+}).use('base', 'test', 'console', 'node', 'node-event-simulate',
+ 'lp.app.widgets.expander', function(Y) {
+
+ function FakeAnim(config) {
+ FakeAnim.superclass.constructor.apply(this, arguments);
+ this.call_stack = [];
+ }
+
+ FakeAnim.ATTRS = {
+ running: { value: false },
+ reverse: { value: false },
+ to: { value: {} },
+ from: { value: {} },
+ };
+
+ Y.extend(FakeAnim, Y.Base, {
+ stop: function() {
+ this.call_stack.push('stop');
+ this.set('running', false);
+ },
+
+ run: function() {
+ this.call_stack.push('run');
+ this.set('running', true);
+ }
+ });
var suite = new Y.Test.Suite("lp.app.widgets.expander Tests");
var module = Y.lp.app.widgets.expander;
@@ -144,6 +168,19 @@
Y.Assert.isTrue(render_has_run);
},
+ test_setUp_calls_foldContentNode_no_anim: function() {
+ var foldContentNode_animate_arg = false;
+ var fake_foldContentNode = function(expanded, no_animate) {
+ foldContentNode_animate_arg = no_animate;
+ };
+ var old_method = module.Expander.prototype.foldContentNode;
+ module.Expander.prototype.foldContentNode = fake_foldContentNode;
+ var expander = this.makeExpander();
+ expander.foldContentNode = fake_foldContentNode;
+ Y.Assert.isTrue(foldContentNode_animate_arg);
+ module.Expander.prototype.foldContentNode = old_method;
+ },
+
test_createByCSS_creates_expander: function() {
var root = this.makeExpanderHooks();
this.findTestHookTag().appendChild(root);
@@ -182,7 +219,159 @@
var children = expander.content_node.get('children');
Y.Assert.areEqual(1, children.size());
Y.Assert.areEqual(ajax_result, children.item(0));
- }
+ },
+
+ test_receive_success_leaves_loaded: function() {
+ var expander = this.makeExpander();
+ Y.Assert.isTrue(expander.loaded);
+ expander.receive('');
+ Y.Assert.isTrue(expander.loaded);
+ },
+
+ test_receive_failure_resets_loaded: function() {
+ var expander = this.makeExpander();
+ Y.Assert.isTrue(expander.loaded);
+ expander.receive('', true);
+ Y.Assert.isFalse(expander.loaded);
+ },
+
+ test_receive_stops_and_restarts_animation: function() {
+ var expander = this.makeExpander();
+ var anim = new FakeAnim();
+ anim.set('running', true);
+ expander._animation = anim;
+ expander.receive('');
+ // Animation is first stopped, then restarted with run().
+ Y.ArrayAssert.itemsAreSame(
+ ['stop', 'run'], anim.call_stack);
+ },
+
+ test_receive_restarts_at_current_height: function() {
+ var expander = this.makeExpander();
+
+ var anim = new FakeAnim();
+ expander._animation = anim;
+
+ // We've got a half (well, 40%) open container node
+ // with current height at 2px.
+ var content_node = Y.Node.create('<div />')
+ .setStyle('height', '2px');
+ this.findTestHookTag().appendChild(content_node);
+ expander.content_node = content_node;
+
+ // Full desired content height of 5px.
+ var content = Y.Node.create('<div />')
+ .setStyle('height', '5px');
+
+ expander.receive(content);
+ // We get an integer from scrollHeight, and pixels from height.
+ Y.Assert.areEqual(5, anim.get('to').height);
+ Y.Assert.areEqual('2px', anim.get('from').height);
+ },
+
+ test_foldContentNode_expand_no_animation: function() {
+ var expander = this.makeExpander();
+
+ var anim = new FakeAnim();
+ expander._animation = anim;
+
+ // First parameter is true for expand, false for folding.
+ // Second parameter indicates if no animation should be used
+ // (true for no animation, anything else otherwise).
+ expander.foldContentNode(true, true);
+
+ // No anim.run() calls have been executed.
+ Y.ArrayAssert.itemsAreEqual([], anim.call_stack);
+ // And unseen CSS class has been removed.
+ Y.Assert.isFalse(
+ expander.content_node.hasClass("unseen"));
+ },
+
+ test_foldContentNode_fold_no_animation: function() {
+ var expander = this.makeExpander();
+
+ var anim = new FakeAnim();
+ expander._animation = anim;
+
+ // First parameter is true for expand, false for folding.
+ // Second parameter indicates if no animation should be used
+ // (true for no animation, anything else otherwise).
+ expander.foldContentNode(false, true);
+
+ // No anim.run() calls have been executed.
+ Y.ArrayAssert.itemsAreEqual([], anim.call_stack);
+ // And unseen CSS class has been added.
+ Y.Assert.isTrue(
+ expander.content_node.hasClass("unseen"));
+ },
+
+ test_foldContentNode_expand: function() {
+ // Expanding a content node sets the animation direction
+ // as appropriate ('reverse' to false) and removes the
+ // 'unseen' CSS class.
+ var expander = this.makeExpander();
+
+ var anim = new FakeAnim();
+ anim.set('reverse', true);
+ expander._animation = anim;
+
+ expander.foldContentNode(true);
+
+ // Reverse flag has been toggled.
+ Y.Assert.isFalse(anim.get('reverse'));
+ // 'unseen' CSS class has been removed.
+ Y.Assert.isFalse(expander.content_node.hasClass("unseen"));
+ // Animation is shown.
+ Y.ArrayAssert.itemsAreEqual(['run'], anim.call_stack);
+ },
+
+ test_foldContentNode_fold: function() {
+ // Folding a content node sets the animation direction
+ // as appropriate ('reverse' to false) and removes the
+ // 'unseen' CSS class.
+ var expander = this.makeExpander();
+
+ var anim = new FakeAnim();
+ anim.set('reverse', true);
+ expander._animation = anim;
+ // Initially expanded (with no animation).
+ expander.foldContentNode(true, true);
+
+ // Now fold it back.
+ expander.foldContentNode(false);
+
+ // Reverse flag has been toggled.
+ Y.Assert.isTrue(anim.get('reverse'));
+ // Animation is shown.
+ Y.ArrayAssert.itemsAreEqual(['run'], anim.call_stack);
+ // 'unseen' CSS class is added back, but only when
+ // the animation completes.
+ Y.Assert.isFalse(expander.content_node.hasClass("unseen"));
+ anim.fire('end');
+ Y.Assert.isTrue(expander.content_node.hasClass("unseen"));
+ },
+
+ test_foldContentNode_fold_expand: function() {
+ // Quickly folding then re-expanding a node doesn't
+ // set the 'unseen' flag.
+ var expander = this.makeExpander();
+ var anim = new FakeAnim();
+ anim.set('reverse', true);
+ expander._animation = anim;
+ // Initially expanded (with no animation).
+ expander.foldContentNode(true, true);
+
+ // Now fold it.
+ expander.foldContentNode(false);
+ Y.Assert.isFalse(expander.content_node.hasClass("unseen"));
+ // And expand it before animation was completed.
+ expander.foldContentNode(true);
+ // When animation for folding completes, it does not
+ // set the 'unseen' CSS class because expanding is now
+ // in progress instead.
+ anim.fire('end');
+ Y.Assert.isFalse(expander.content_node.hasClass("unseen"));
+ },
}));
var handle_complete = function(data) {
Follow ups
