launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05333
[Merge] lp:~abentley/launchpad/display-cleanup into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/display-cleanup into lp:launchpad with lp:~abentley/launchpad/navigate-batches as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #882795 in Launchpad itself: "ajax bug listing navigation needs polish"
https://bugs.launchpad.net/launchpad/+bug/882795
For more details, see:
https://code.launchpad.net/~abentley/launchpad/display-cleanup/+merge/80621
= Summary =
Fix bug #882795: ajax bug listing navigation needs polish
== Proposed fix ==
Replace all navigation nodes with hyperlink nodes.
Mark first/previous disabled on first batch.
Mark last/next disabled on last batch.
Update batch info (1 -> 5 of 10) on navigation.
== Pre-implementation notes ==
None
== Implementation details ==
None
== Tests ==
bin/test -t test_buglisting.html
== Demo and Q/A ==
- Go to a listing page. first & previous should be disabled.
- Click last. next & last should be disabled, and first & previous should be green. The info about the batch should be updated.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/javascript/buglisting.js
lib/lp/bugs/templates/bugs-listing-table.pt
lib/lp/bugs/templates/buglisting-default.pt
lib/lp/bugs/browser/tests/test_bugtask.py
lib/lp/bugs/browser/bugtask.py
lib/lp/bugs/templates/bugs-listing-table-without-navlinks.pt
lib/lp/bugs/javascript/tests/test_buglisting.js
--
https://code.launchpad.net/~abentley/launchpad/display-cleanup/+merge/80621
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/display-cleanup into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-10-27 21:03:30 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-10-27 21:03:31 +0000
@@ -2495,6 +2495,7 @@
cache.objects['next'] = _getBatchInfo(next_batch)
prev_batch = batch_navigator.batch.prevBatch()
cache.objects['prev'] = _getBatchInfo(prev_batch)
+ cache.objects['total'] = batch_navigator.batch.total()
cache.objects['order_by'] = ','.join(
get_sortorder_from_request(self.request))
cache.objects['forwards'] = batch_navigator.batch.range_forwards
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2011-10-27 21:03:30 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2011-10-27 21:03:31 +0000
@@ -1424,6 +1424,7 @@
self.assertIs(None, cache.objects['memo'])
self.assertEqual(0, cache.objects['start'])
self.assertTrue(cache.objects['forwards'])
+ self.assertEqual(1, cache.objects['total'])
def test_cache_has_all_batch_vars_specified(self):
"""Cache has all the needed variables.
=== modified file 'lib/lp/bugs/javascript/buglisting.js'
--- lib/lp/bugs/javascript/buglisting.js 2011-10-27 21:03:30 +0000
+++ lib/lp/bugs/javascript/buglisting.js 2011-10-27 21:03:31 +0000
@@ -18,11 +18,13 @@
* cache is the JSONRequestCache for the batch.
* template is the template to use for rendering batches.
* target is a YUI node to update when rendering batches.
+ * navigation_indices is a YUI NodeList of nodes to update with the current
+ * batch info.
* io_provider is something providing the Y.io interface, typically used for
* testing. Defaults to Y.io.
*/
namespace.ListingNavigator = function(current_url, cache, template, target,
- io_provider) {
+ navigation_indices, io_provider) {
var lp_client = new Y.lp.client.Launchpad();
this.search_params = namespace.get_query(current_url);
delete this.search_params.start;
@@ -34,6 +36,14 @@
this.template = template;
this.target = target;
this.batches = {};
+ this.backwards_navigation = new Y.NodeList([]);
+ this.forwards_navigation = new Y.NodeList([]);
+ if (!Y.Lang.isValue(navigation_indices)){
+ navigation_indices = new Y.NodeList([]);
+ }
+ this.navigation_indices = navigation_indices;
+ this.batch_info_template = '<strong>{{start}}</strong> → ' +
+ '<strong>{{end}}</strong> of {{total}} results';
};
@@ -53,18 +63,38 @@
nodes.addClass('js-action');
};
+/**
+ * Rewrite all nodes with navigation classes so that they are hyperlinks.
+ * Content is retained.
+ */
+namespace.linkify_navigation = function(){
+ Y.each(['previous', 'next', 'first', 'last'], function(class_name){
+ Y.all('.' + class_name).each(function(node){
+ new_node = Y.Node.create('<a href="#"></a>');
+ new_node.addClass(class_name);
+ new_node.setContent(node.getContent());
+ node.replace(new_node);
+ });
+ });
+};
/**
* Factory to return a ListingNavigator for the given page.
*/
namespace.ListingNavigator.from_page = function(){
var target = Y.one('#client-listing');
+ var navigation_indices = Y.all('.batch-navigation-index');
var navigator = new namespace.ListingNavigator(
- window.location, LP.cache, LP.mustache_listings, target);
+ window.location, LP.cache, LP.mustache_listings, target,
+ navigation_indices);
+ namespace.linkify_navigation();
+ navigator.backwards_navigation = Y.all('.first,.previous');
+ navigator.forwards_navigation = Y.all('.last,.next');
navigator.clickAction('.first', navigator.first_batch);
navigator.clickAction('.next', navigator.next_batch);
navigator.clickAction('.previous', navigator.prev_batch);
navigator.clickAction('.last', navigator.last_batch);
+ navigator.render_navigation();
return navigator;
};
@@ -77,13 +107,31 @@
*
* The template is always LP.mustache_listings.
*/
-namespace.ListingNavigator.prototype.rendertable = function(){
+namespace.ListingNavigator.prototype.render = function(){
if (! Y.Lang.isValue(this.target)){
return;
}
var model = this.current_batch.mustache_model;
- var txt = Mustache.to_html(this.template, model);
- this.target.set('innerHTML', txt);
+ var batch_info = Mustache.to_html(this.batch_info_template, {
+ start: this.current_batch.start + 1,
+ end: this.current_batch.start +
+ this.current_batch.mustache_model.bugtasks.length + 1,
+ total: this.current_batch.total
+ });
+ this.target.setContent(Mustache.to_html(this.template, model));
+ this.navigation_indices.setContent(batch_info);
+ this.render_navigation();
+};
+
+
+/**
+ * Enable/disable navigation links as appropriate.
+ */
+namespace.ListingNavigator.prototype.render_navigation = function(){
+ this.backwards_navigation.toggleClass(
+ 'inactive', this.current_batch.prev === null);
+ this.forwards_navigation.toggleClass(
+ 'inactive', this.current_batch.next === null);
};
@@ -105,7 +153,7 @@
var key = namespace.ListingNavigator.get_batch_key(model);
this.batches[key] = model;
this.current_batch = model;
- this.rendertable();
+ this.render();
};
@@ -139,7 +187,7 @@
var cached_batch = this.batches[key];
if (Y.Lang.isValue(cached_batch)){
this.current_batch = cached_batch;
- this.rendertable();
+ this.render();
}
else {
this.load_model(config);
=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting.js'
--- lib/lp/bugs/javascript/tests/test_buglisting.js 2011-10-27 21:03:30 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting.js 2011-10-27 21:03:31 +0000
@@ -21,26 +21,132 @@
}));
suite.add(new Y.Test.Case({
- name: 'rendertable',
-
- test_rendertable_no_client_listing: function() {
+ name: 'render',
+
+ tearDown: function(){
+ Y.one('#fixture').setContent('');
+ },
+
+ test_render_no_client_listing: function() {
// Rendering should not error with no #client-listing.
var navigator = new module.ListingNavigator();
- navigator.rendertable();
+ navigator.render();
},
- test_rendertable: function() {
- // Rendering should work with #client-listing supplied.
+ get_render_navigator: function() {
var target = Y.Node.create('<div id="client-listing"></div>');
var lp_cache = {
mustache_model: {
- foo: 'bar'
- }
+ foo: 'bar',
+ bugtasks: ['a', 'b', 'c']
+ },
+ next: null,
+ prev: null,
+ start: 5,
+ total: 256
};
var template = "{{foo}}";
- var navigator = new module.ListingNavigator(
+ var navigator = new module.ListingNavigator(
null, lp_cache, template, target);
- navigator.rendertable();
- Y.Assert.areEqual('bar', navigator.target.get('innerHTML'));
+ var index = Y.Node.create(
+ '<div><strong>3</strong> → <strong>4</strong>' +
+ ' of 512 results</div>');
+ navigator.navigation_indices.push(index);
+ navigator.backwards_navigation.push(Y.Node.create('<div></div>'));
+ navigator.forwards_navigation.push(Y.Node.create('<div></div>'));
+ return navigator;
+ },
+ test_render: function() {
+ // Rendering should work with #client-listing supplied.
+ var navigator = this.get_render_navigator();
+ navigator.render();
+ Y.Assert.areEqual('bar', navigator.target.getContent());
+ },
+ /**
+ * render_navigation should disable "previous" and "first" if there is
+ * no previous batch (i.e. we're at the beginning.)
+ */
+ test_render_navigation_disables_backwards_navigation_if_no_prev:
+ function() {
+ var navigator = this.get_render_navigator();
+ var action = navigator.backwards_navigation.item(0);
+ navigator.render_navigation();
+ Y.Assert.isTrue(action.hasClass('inactive'));
+ },
+ /**
+ * render_navigation should enable "previous" and "first" if there is
+ * a previous batch (i.e. we're not at the beginning.)
+ */
+ test_render_navigation_enables_backwards_navigation_if_prev:
+ function() {
+ var navigator = this.get_render_navigator();
+ var action = navigator.backwards_navigation.item(0);
+ action.addClass('inactive');
+ navigator.current_batch.prev = {
+ start: 1, memo: 'pi'
+ };
+ navigator.render_navigation();
+ Y.Assert.isFalse(action.hasClass('inactive'));
+ },
+ /**
+ * render_navigation should disable "next" and "last" if there is
+ * no next batch (i.e. we're at the end.)
+ */
+ test_render_navigation_disables_forwards_navigation_if_no_next:
+ function() {
+ var navigator = this.get_render_navigator();
+ var action = navigator.forwards_navigation.item(0);
+ navigator.render_navigation();
+ Y.Assert.isTrue(action.hasClass('inactive'));
+ },
+ /**
+ * render_navigation should enable "next" and "last" if there is a next
+ * batch (i.e. we're not at the end.)
+ */
+ test_render_navigation_enables_forwards_navigation_if_next: function() {
+ var navigator = this.get_render_navigator();
+ var action = navigator.forwards_navigation.item(0);
+ action.addClass('inactive');
+ navigator.current_batch.next = {
+ start: 1, memo: 'pi'
+ };
+ navigator.render_navigation();
+ Y.Assert.isFalse(action.hasClass('inactive'));
+ },
+ /**
+ * linkify_navigation should convert previous, next, first last into
+ * hyperlinks, while retaining the original content.
+ */
+ test_linkify_navigation: function(){
+ Y.one('#fixture').setContent(
+ '<span class="previous">PreVious</span>' +
+ '<span class="next">NeXt</span>' +
+ '<span class="first">FiRST</span>' +
+ '<span class="last">lAst</span>');
+ module.linkify_navigation();
+ function checkNav(selector, content){
+ var node = Y.one(selector);
+ Y.Assert.areEqual('a', node.get('tagName').toLowerCase());
+ Y.Assert.areEqual(content, node.getContent());
+ Y.Assert.areEqual('#', node.get('href').substr(-1, 1));
+ }
+ checkNav('.previous', 'PreVious');
+ checkNav('.next', 'NeXt');
+ checkNav('.first', 'FiRST');
+ checkNav('.last', 'lAst');
+ },
+ /**
+ * Render should update the navigation_indices with the result info.
+ */
+ test_render_navigation_indices: function(){
+ var navigator = this.get_render_navigator();
+ index = navigator.navigation_indices.item(0);
+ Y.Assert.areEqual(
+ '<strong>3</strong> \u2192 <strong>4</strong> of 512 results',
+ index.getContent());
+ navigator.render();
+ Y.Assert.areEqual(
+ '<strong>6</strong> \u2192 <strong>9</strong> of 256 results',
+ index.getContent());
}
}));
@@ -58,26 +164,29 @@
web_link: 'http://foo/bar'
},
mustache_model: {
- foo: 'bar'
+ foo: 'bar',
+ bugtasks: []
}
};
var target = Y.Node.create('<div id="client-listing"></div>');
var navigator = new module.ListingNavigator(
"http://yahoo.com?start=5&memo=6&direction=backwards";,
lp_cache, "<ol>" + "{{#item}}<li>{{name}}</li>{{/item}}</ol>",
- target, mock_io);
+ target, null, mock_io);
navigator.first_batch('intensity');
- Y.Assert.areEqual('', navigator.target.get('innerHTML'));
+ Y.Assert.areEqual('', navigator.target.getContent());
mock_io.last_request.successJSON({
context: {
resource_type_link: 'http://foo_type',
web_link: 'http://foo/bar'
},
mustache_model:
- {item: [
+ {
+ item: [
{name: 'first'},
- {name: 'second'}
- ]},
+ {name: 'second'}],
+ bugtasks: []
+ },
order_by: 'intensity',
start: 0,
forwards: true,
@@ -91,7 +200,7 @@
var navigator = this.get_intensity_listing();
var mock_io = navigator.io_provider;
Y.Assert.areEqual('<ol><li>first</li><li>second</li></ol>',
- navigator.target.get('innerHTML'));
+ navigator.target.getContent());
Y.Assert.areEqual('/bar/+bugs/++model++?orderby=intensity&start=0',
mock_io.last_request.url);
},
@@ -247,7 +356,8 @@
order_by: 'foo',
last_start: 23
};
- return new module.ListingNavigator(url, lp_cache, null, null, mock_io);
+ return new module.ListingNavigator(url, lp_cache, null, null, null,
+ mock_io);
};
suite.add(new Y.Test.Case({
