launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04190
[Merge] lp:~danilo/launchpad/replace-expanders-1 into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/replace-expanders-1 into lp:launchpad with lp:~danilo/launchpad/expander-anim as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~danilo/launchpad/replace-expanders-1/+merge/67169
= Replace few expanders =
We replace a few one-off expander implementations in the code with our new streamlined expander widget.
== Implementation details ==
This extends Expander widget to allow having a separate animation node (useful for tables because TRs can't have a height lesser than what the minimum is required for displaying full content, when you can animate a node inside the TR > TD, and still set 'unseen' CSS flag on the TR), and allows directly linkifying the icon node and setting js-action as needed.
Tests are provided for these changes.
For the rest of the JS, inline in actual templates, no tests are provided other than the potentially existing ones (will find out about them in a full test suite run). I don't want to spend time implementing too many tests where there were none before because I'd much rather spend time making all of our code use the new single expander so people looking for examples would hit The Right Way.
The new expander should be much nicer and should be smart when you try quickly toggling it (no jerking should ever happen, unless you have a very slow computer). Tested on Epiphany (bare WebKit, like Safari ;), Firefox, Chromium.
== Tests ==
lib/lp/app/javascript/tests/test_expander.html
(and existing tests where appropriate)
== Demo and Q/A ==
Bug task expanders:
https://bugs.launchpad.dev/firefox/+bug/1
Package details (loaded over ajax):
https://launchpad.dev/~mark/+archive/ppa/+packages
https://launchpad.net/~danilo/+archive/epiphany/+packages
PPA details:
https://launchpad.dev/~mark/+archive/ppa
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/expander.js
lib/lp/app/javascript/tests/test_expander.js
lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt
lib/lp/bugs/templates/bugtasks-and-nominations-table.pt
lib/lp/soyuz/templates/archive-index.pt
lib/lp/soyuz/templates/archive-macros.pt
lib/lp/soyuz/templates/sourcepackagepublishinghistory-listing-archive-detailed.pt
--
https://code.launchpad.net/~danilo/launchpad/replace-expanders-1/+merge/67169
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/replace-expanders-1 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/expander.js'
--- lib/lp/app/javascript/expander.js 2011-07-07 11:22:30 +0000
+++ lib/lp/app/javascript/expander.js 2011-07-07 11:22:31 +0000
@@ -50,6 +50,9 @@
* "expander" as its argument. Once the loader has constructed the
* output Node or NodeList it wants to display ("output"), it calls
* expander.receive(output) to update the content node.
+ * animate_node: A node to perform an animation on. Mostly useful for
+ * animating table rows/cells when you want to animate the inner
+ * content (eg. a <div>).
*/
function Expander(icon_node, content_node, config) {
if (!Y.Lang.isObject(icon_node)) {
@@ -66,8 +69,14 @@
this.config = {};
}
this.loaded = !Y.Lang.isValue(this.config.loader);
+
+ if (Y.Lang.isValue(this.config.animate_node)) {
+ this._animate_node = Y.one(this.config.animate_node);
+ } else {
+ this._animate_node = this.content_node;
+ }
this._animation = Y.lazr.effects.reversible_slide_out(
- this.content_node);
+ this._animate_node);
// Is setup complete? Skip any animations until it is.
this.fully_set_up = false;
@@ -162,7 +171,7 @@
revealIcon: function() {
this.icon_node
- .addClass('sprite').addClass('js-action')
+ .addClass('sprite')
.removeClass('unseen');
},
@@ -189,9 +198,9 @@
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');
+ var from_height = this._animate_node.getStyle('height');
+ this._animate_node.setContent(output);
+ var to_height = this._animate_node.get('scrollHeight');
if (this._animation.get('running')) {
this._animation.stop();
}
@@ -224,11 +233,28 @@
this.setExpanded(expanded);
},
+ /**
+ * Wrap node content in an <a> tag and mark it as js-action.
+ *
+ * @param node Y.Node object to modify: its content is modified
+ * in-place so node events won't be lost, but anything set on
+ * the inner content nodes might be.
+ */
+ wrapNodeWithLink: function(node) {
+ var link_node = Y.Node.create('<a></a>')
+ .addClass('js-action')
+ .set('href', '#')
+ .setContent(node.getContent());
+ node.setContent(link_node);
+ },
+
/*
* Set up an expander's DOM and event handler.
*
+ * @param linkify {Boolean} Wrap the icon node into an <A> tag with
+ * proper CSS classes and content from the icon node.
*/
- setUp: function() {
+ setUp: function(linkify) {
var expander = this;
function click_handler(e) {
e.halt();
@@ -236,6 +262,9 @@
}
this.render(this.isExpanded(), true);
+ if (linkify === true) {
+ this.wrapNodeWithLink(this.icon_node);
+ }
this.icon_node.on('click', click_handler);
this.revealIcon();
this.fully_set_up = true;
=== modified file 'lib/lp/app/javascript/tests/test_expander.js'
--- lib/lp/app/javascript/tests/test_expander.js 2011-07-07 11:22:30 +0000
+++ lib/lp/app/javascript/tests/test_expander.js 2011-07-07 11:22:31 +0000
@@ -80,6 +80,16 @@
root.one('.icon'), root.one('.content'), args.config).setUp();
},
+ test_separate_animate_node: function() {
+ var icon = Y.Node.create('<td></td>'),
+ content = Y.Node.create('<td></td>'),
+ animate = Y.Node.create('<div></div>');
+ var expander = new module.Expander(icon, content,
+ { animate_node: animate });
+ Y.Assert.areSame(content, expander.content_node);
+ Y.Assert.areSame(animate, expander._animation.get('node'));
+ },
+
test_loaded_is_true_if_no_loader_is_defined: function() {
var icon = Y.Node.create('<p></p>'),
content = Y.Node.create('<p></p>');
@@ -115,7 +125,6 @@
icon.addClass('unseen');
var expander = this.makeExpander(root);
Y.Assert.isFalse(icon.hasClass('unseen'));
- Y.Assert.isTrue(icon.hasClass('js-action'));
},
test_setUp_hides_content_by_default: function() {
@@ -168,6 +177,21 @@
Y.Assert.isTrue(render_has_run);
},
+ test_setUp_linkifies_when_asked: function() {
+ var wrap_has_run = false;
+ var fake_wrapNodeWithLink = function() {
+ wrap_has_run = true;
+ };
+
+ root = this.makeExpanderHooks();
+ var expander = new module.Expander(
+ root.one('.icon'), root.one('.content'));
+ expander.wrapNodeWithLink = fake_wrapNodeWithLink;
+
+ expander.setUp(true);
+ Y.Assert.isTrue(wrap_has_run);
+ },
+
test_setUp_calls_foldContentNode_no_anim: function() {
var foldContentNode_animate_arg = false;
var fake_foldContentNode = function(expanded, no_animate) {
@@ -257,7 +281,7 @@
var content_node = Y.Node.create('<div />')
.setStyle('height', '2px');
this.findTestHookTag().appendChild(content_node);
- expander.content_node = content_node;
+ expander.content_node = expander._animate_node = content_node;
// Full desired content height of 5px.
var content = Y.Node.create('<div />')
@@ -371,6 +395,21 @@
// in progress instead.
anim.fire('end');
Y.Assert.isFalse(expander.content_node.hasClass("unseen"));
+ },
+
+ test_wrapNodeWithLink: function() {
+ // Wraps node content with an <a href="#" class="js-action"> tag.
+ var node = Y.Node.create('<div></div>')
+ .set('text', 'Test');
+ var expander = this.makeExpander();
+ expander.wrapNodeWithLink(node);
+ var link_node = node.one('a');
+ Y.Assert.isNotNull(link_node);
+ Y.Assert.areSame('Test', link_node.get('text'));
+ Y.Assert.isTrue(link_node.hasClass('js-action'));
+ // Link href is '#', but we get full test path appended
+ // so instead we just ensure we've got a string back.
+ Y.Assert.isString(link_node.get('href'));
}
}));
=== modified file 'lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt'
--- lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt 2011-04-14 16:37:22 +0000
+++ lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt 2011-07-07 11:22:31 +0000
@@ -9,17 +9,14 @@
form_row_id string:task${context/id}">
<tr tal:define="editstatus_url string:${context/fmt:url}/+editstatus"
tal:attributes="class view/getTaskRowCSSClass; id row_id">
- <td class="icon left right">
+ <td>
<metal:expander
metal:define-macro="expander"
tal:define="expander_link_text expander_link_text|nothing">
<a tal:condition="expandable"
- tal:attributes="
- href tasklink;
- onclick string:return toggleFormVisibility('${form_row_id}')"
- class="sprite bug-status-expand">
+ tal:attributes="href tasklink" class="bugtask-expander">
+ <tal:link_text tal:replace="expander_link_text" />
- <tal:link_text tal:replace="expander_link_text" />
</a>
<tal:no_expander tal:condition="not: expandable"
tal:replace="expander_link_text" />
@@ -175,19 +172,27 @@
class="bugtasks-table-row-init-script"
tal:condition="not:view/many_bugtasks"
tal:content="string:
- LPS.use('event', 'lp.bugs.bugtask_index', function(Y) {
- Y.on('load',
- function(e) {
- Y.lp.bugs.bugtask_index.setup_bugtask_row(${view/js_config});
- },
- window);
+ LPS.use('event', 'lp.bugs.bugtask_index', 'lp.app.widgets.expander',
+ function(Y) {
+ Y.on('domready', function() {
+ Y.lp.bugs.bugtask_index.setup_bugtask_row(${view/js_config});
+
+ // Set-up the expander on the bug task summary row.
+ var icon_node = Y.one('tr#${row_id} a.bugtask-expander');
+ var row_node = Y.one('tr#${form_row_id}');
+ var content_node = row_node.one('td form');
+ var expander = new Y.lp.app.widgets.expander.Expander(
+ icon_node, row_node, { animate_node: content_node });
+ expander.setUp();
+ });
});
"/>
+
<tal:form condition="view/displayEditForm">
<tr
tal:attributes="id form_row_id"
tal:condition="expandable"
- style="display: none"
+ class="bugtask-collapsible-content unseen"
>
<td colspan="7" tal:content="structure view/edit_view" />
</tr>
=== modified file 'lib/lp/bugs/templates/bugtasks-and-nominations-table.pt'
--- lib/lp/bugs/templates/bugtasks-and-nominations-table.pt 2010-11-01 14:29:57 +0000
+++ lib/lp/bugs/templates/bugtasks-and-nominations-table.pt 2011-07-07 11:22:31 +0000
@@ -59,19 +59,6 @@
</tal:not-editable>
</tal:affects-me-too>
-<script type="text/javascript">
- function toggleFormVisibility(row_id) {
- row = document.getElementById(row_id)
- edit_icon = document.getElementById(row_id + "-edit")
- if (row.style.display=="none") {
- row.style.display = "table-row";
- } else {
- row.style.display = "none";
- }
- return false;
- }
-</script>
-
<table
id="affected-software"
tal:attributes="class python: context.duplicateof and 'duplicate listing' or 'listing'"
=== modified file 'lib/lp/soyuz/templates/archive-index.pt'
--- lib/lp/soyuz/templates/archive-index.pt 2011-05-16 19:36:21 +0000
+++ lib/lp/soyuz/templates/archive-index.pt 2011-07-07 11:22:31 +0000
@@ -95,10 +95,8 @@
target="help">Read about installing</a>)
</p>
- <div id="ppa-install-slide-trigger">
- <div class="widget-header">
- Technical details about this PPA
- </div>
+ <div class="widget-header">
+ Technical details about this PPA
</div>
<div class="widget-body">
@@ -148,34 +146,13 @@
>Celso Providelo</a>.</p>
<script type="text/javascript">
- LPS.use('node', 'event', 'lazr.effects', function(Y) {
-
- // Hide the widget body contents.
- Y.one('#ppa-install .widget-body').addClass('lazr-closed');
-
- // Ensure that the widget header uses the correct sprite icon
- // and gets the styling for javascript actions applied.
- var widget_header = Y.one('#ppa-install .widget-header');
- widget_header.addClass('sprite');
- widget_header.addClass('treeCollapsed');
- widget_header.addClass('js-action');
-
- var slide;
- function toggle_body_visibility(e) {
- if (!slide) {
- slide = Y.lazr.effects.slide_out('#ppa-install .widget-body');
- widget_header.replaceClass('treeCollapsed', 'treeExpanded');
- } else {
- slide.set('reverse', !slide.get('reverse'));
- widget_header.toggleClass('treeExpanded');
- widget_header.toggleClass('treeCollapsed');
- }
- slide.stop();
- slide.run();
- }
- Y.on('click', toggle_body_visibility,
- '#ppa-install-slide-trigger');
-});
+ LPS.use('lp.app.widgets.expander', function(Y) {
+ var widget_header = Y.one('#ppa-install .widget-header');
+ var content = Y.one('.widget-body');
+ var expander = new Y.lp.app.widgets.expander.Expander(
+ widget_header, content);
+ expander.setUp(true);
+ });
</script>
</tal:is-active>
=== modified file 'lib/lp/soyuz/templates/archive-macros.pt'
--- lib/lp/soyuz/templates/archive-macros.pt 2011-04-08 14:59:14 +0000
+++ lib/lp/soyuz/templates/archive-macros.pt 2011-07-07 11:22:31 +0000
@@ -10,7 +10,8 @@
</tal:comment>
<script type="text/javascript">
-LPS.use('node', 'io-base', 'lazr.anim', 'lp.soyuz.base', function(Y) {
+LPS.use('node', 'io-base', 'lazr.anim', 'lp.soyuz.base',
+ 'lp.app.widgets.expander', function(Y) {
/*
@@ -18,19 +19,13 @@
* the operation.
*/
function informFailure(transaction_id, response, args) {
- function retry_handler(e) {
- e.preventDefault();
- startUpdate(args.container);
- };
-
var failure_message = Y.lp.soyuz.base.makeFailureNode(
- 'Failed to fetch package details.', retry_handler);
+ 'Failed to fetch package details.');
- args.container.set('innerHTML', '');
- args.container.appendChild(failure_message);
+ args.expander.receive(failure_message);
var anim = Y.lazr.anim.red_flash({
- node: args.container
+ node: args.expander.content_node
});
anim.run();
}
@@ -40,19 +35,16 @@
* Update the row with the XHR response.
*/
function doUpdate(transaction_id, response, args) {
- args.container.set('innerHTML', response.responseText);
+ var node = Y.Node.create('<div />')
+ .set('innerHTML', response.responseText);
+ args.expander.receive(node);
}
-/*
- * Dispatch a XHR for updating the given container.
+/**
+ * Dispatch a XHR to load the given container.
*/
-function startUpdate(container) {
- var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
- 'Fetching package details ...')
-
- container.set('innerHTML', '');
- container.appendChild(in_progress_message);
+function loadDetails(expander) {
var config = {
on: {
@@ -60,11 +52,11 @@
'failure': informFailure
},
arguments: {
- 'container': container,
+ 'expander': expander
}
};
- /*
+ /**
* If a page wants to use this outside of an archive context then it
* can define LP.cache['archive_context'], which should be a
* full or relative URL to the context archive page required.
@@ -74,63 +66,37 @@
? LP.cache['archive_context_url']
: '';
if (base_url !== '') {
- base_url += '/'
+ base_url += '/';
}
- var pub_id = container.get('id').replace(
- '-container', '').replace('pub', '');
+ var pub_id = expander.content_node.get('id')
+ .replace('pub', '')
+ .replace('-container', '');
var uri = base_url + '+sourcepub/' + pub_id + '/+listing-archive-extra';
Y.io(uri, config);
}
-
-/*
- * Toggle the visibility of the expander targeted and the visual of
- * the expander itself.
- */
-function toggleExpandableRow(expander) {
- var row = Y.one('#' + expander.get('id').replace('-expander', ''));
- var icon = expander.one('img');
-
- var row_display = row.getStyle('display');
- if (row_display == 'none') {
- row.setStyle('display', 'table-row');
- icon.set('src', '/@@/treeExpanded');
- var container = Y.one('#' + row.get('id') + '-container');
- if (trim(container.get('innerHTML')) == ''){
- startUpdate(container);
- }
- }
- else {
- row.setStyle('display', 'none');
- icon.set('src', '/@@/treeCollapsed');
- }
-}
-
-
-/*
- * Setup expander handlers and pre-load 'treeExpanded', 'no' and
- * 'spinner' images for better reponsiveness.
+/**
+ * Setup expander handlers.
*/
function setupPackageListExpanders() {
- var spinner = new Image();
- spinner.src = '/@@/spinner';
-
- var no = new Image();
- no.src = '/@@/no';
-
- var tree_expanded = new Image();
- tree_expanded.src = '/@@/treeExpanded';
-
- function expander_handler (e) {
- e.preventDefault();
- toggleExpandableRow(e.currentTarget);
- };
-
- // Note: there are situations, such as displaying empty result
- // sets, when there will not be any links for expanders on the page.
var expanders = Y.all('#packages_list a.expander');
- if (expanders !== null){
- expanders.on("click", expander_handler);
+ var widget;
+ if (expanders !== null) {
+ function setupExpander(expander) {
+ var base_id = expander.get('id').replace('-expander', '');
+ var container = Y.one('#' + base_id);
+ var content = container.one('td div.package-details');
+ var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
+ 'Fetching package details ...');
+ content.empty();
+ content.appendChild(in_progress_message);
+
+ widget = new Y.lp.app.widgets.expander.Expander(
+ expander, container, { loader: loadDetails,
+ animate_node: content });
+ widget.setUp();
+ };
+ expanders.each(setupExpander);
}
}
=== modified file 'lib/lp/soyuz/templates/sourcepackagepublishinghistory-listing-archive-detailed.pt'
--- lib/lp/soyuz/templates/sourcepackagepublishinghistory-listing-archive-detailed.pt 2010-05-17 13:14:46 +0000
+++ lib/lp/soyuz/templates/sourcepackagepublishinghistory-listing-archive-detailed.pt 2011-07-07 11:22:31 +0000
@@ -13,11 +13,10 @@
tal:condition="view/allow_selection"
tal:attributes="value context/id;
id string:field.selected_sources.${context/id}"/>
- <a class="expander"
- tal:attributes="id string:pub${context/id}-expander;
- href string:+sourcepub/${context/id}/+listing-archive-extra">
- <img src="/@@/treeCollapsed" alt="Show details" title="Show details"
- tal:attributes="id string:pub${context/id}-arrow" />
+ <a tal:attributes="
+ href string:+sourcepub/${context/id}/+listing-archive-extra;
+ id string:pub${context/id}-expander"
+ class="sprite expander">
<img src="/@@/package-source" />
<tal:source_name replace="context/sourcepackagerelease/title">
foo - 1.0
@@ -69,8 +68,8 @@
</tal:related_builds>
</td>
</tr>
- <tr tal:attributes="id string:pub${context/id}" style="display: none">
- <td colspan="7">
+ <tr tal:attributes="id string:pub${context/id}">
+ <td colspan="7" style="padding: 0px;">
<div class="package-details"
tal:attributes="id string:pub${context/id}-container" />
</td>
