launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04202
[Merge] lp:~danilo/launchpad/replace-expanders-2 into lp:launchpad
Данило Шеган has proposed merging lp:~danilo/launchpad/replace-expanders-2 into lp:launchpad with lp:~danilo/launchpad/drop-collapsibles as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #807434 in Launchpad itself: "Replace source package files, publishing history and similar bug expanders"
https://bugs.launchpad.net/launchpad/+bug/807434
For more details, see:
https://code.launchpad.net/~danilo/launchpad/replace-expanders-2/+merge/67314
= Bug 807434: source package, publishing history and similar bug expanders =
As part of our Thunderdome effort, we are replacing multiple implementations of expanding/collapsing widgets with one implementation to rule them all. This branch does so for another three one-off implementations, simplifying the code in the process.
One is a simple move from toggleExpandableRow (which is removed) to new Expander which includes a nice animation. We also move JS away from the view into the template. This is the sourcepackage-publishinghistory changes.
Another is a move from toggleExpandableRows (removed as well) to the new Expander. Unfortunately, because it shows/hides multiple TRs at the same time, providing animation for them is slightly harder. (In theory, one would have to wrap all cell contents in block-level elements such as DIVs, and then animate such that we first animate the first row until it is fully shown, then the next one, and so forth). Instead, I extend the Expander to allow requesting no animation (this basically cuts down on the duration to hide the rows more than anything else, because animation on TRs behaves basically like hide/show).
And finally, expanders for similar bugs on the +filebug page had a seemingly nice animation but which was easy to break (just try clicking quickly three times pm any of the expander icons on https://bugs.launchpad.net/unity/+filebug when you search for "a"). We replace this one as well (mostly code removal), and fix the test setup to match the new HTML.
I feel lazy to fix all the lint issues which seem unrelated to my code and are in sensitive areas (like LPClient implementation).
== Tests ==
lib/lp/app/javascript/tests/test_expander.html
lib/lp/bugs/javascript/tests/test_filebug_dupfinder.html
== Demo and Q/A ==
https://bugs.launchpad.dev/ubuntu/warty/+queue?queue_state=3&queue_text=
https://launchpad.dev/ubuntu/+source/mozilla-firefox/4.1.2-1ubuntu1/+publishinghistory
https://bugs.launchpad.dev/firefox/+filebug (search for "a", look at the similar bugs section, "Extra options" in the file bug form is already migrated in another branch)
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/expander.js
lib/lp/app/javascript/lp.js
lib/lp/app/javascript/tests/test_expander.js
lib/lp/bugs/javascript/filebug_dupefinder.js
lib/lp/bugs/javascript/tests/test_filebug_dupfinder.js
lib/lp/bugs/templates/bugtarget-macros-filebug.pt
lib/lp/soyuz/browser/publishing.py
lib/lp/soyuz/templates/distributionsourcepackage-publishinghistory.pt
lib/lp/soyuz/templates/distroseries-queue.pt
lib/lp/soyuz/templates/publishinghistory-macros.pt
./lib/lp/app/javascript/lp.js
52: Expected '!==' and instead saw '!='.
108: Move 'var' declarations to the top of the function.
108: Stopping. (50% scanned).
-1: JSLINT had a fatal error.
./lib/lp/bugs/javascript/filebug_dupefinder.js
127: Expected '!==' and instead saw '!='.
149: Expected '===' and instead saw '=='.
395: Expected '===' and instead saw '=='.
336: Line exceeds 78 characters.
./lib/lp/bugs/javascript/tests/test_filebug_dupfinder.js
35: Avoid 'arguments.callee'.
36: Avoid 'arguments.callee'.
39: Avoid 'arguments.callee'.
41: Avoid 'arguments.callee'.
42: Avoid 'arguments.callee'.
149: Unexpected ','.
./lib/lp/soyuz/browser/publishing.py
161: Line exceeds 78 characters.
163: Line exceeds 78 characters.
169: Line exceeds 78 characters.
125: E303 too many blank lines (2)
163: E501 line too long (80 characters)
306: E203 whitespace before ':'
334: W291 trailing whitespace
396: W391 blank line at end of file
make: *** [lint] Error 22
--
https://code.launchpad.net/~danilo/launchpad/replace-expanders-2/+merge/67314
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/replace-expanders-2 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/expander.js'
--- lib/lp/app/javascript/expander.js 2011-07-08 11:27:17 +0000
+++ lib/lp/app/javascript/expander.js 2011-07-08 11:27:23 +0000
@@ -53,6 +53,9 @@
* 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>).
+ * no_animation: Set to true if no animation should be used. Useful for
+ * when you can't rearrange the nodes so animations apply to them
+ * (eg. we want to show a bunch of rows in the same table).
*/
function Expander(icon_node, content_node, config) {
if (!Y.Lang.isObject(icon_node)) {
@@ -75,8 +78,13 @@
} else {
this._animate_node = this.content_node;
}
- this._animation = Y.lazr.effects.reversible_slide_out(
- this._animate_node);
+
+ if (this.config.no_animation !== true) {
+ this._animation = Y.lazr.effects.reversible_slide_out(
+ this._animate_node);
+ } else {
+ this._animation = undefined;
+ }
// Is setup complete? Skip any animations until it is.
this.fully_set_up = false;
@@ -142,10 +150,12 @@
foldContentNode: function(expand, no_animation) {
var expander = this;
var has_paused = false;
- if (no_animation === true) {
+ if (no_animation === true || Y.Lang.isUndefined(this._animation)) {
// Make the animation have the proper direction set from
// the start.
- this._animation.set('reverse', expand);
+ if (!Y.Lang.isUndefined(this._animation)) {
+ this._animation.set('reverse', expand);
+ }
expander.setContentClassIf(
!expand, expander.css_classes.unseen);
} else {
@@ -203,6 +213,9 @@
}
var from_height = this._animate_node.getStyle('height');
this._animate_node.setContent(output);
+ if (Y.Lang.isUndefined(this._animation)) {
+ return;
+ }
var to_height = this._animate_node.get('scrollHeight');
if (this._animation.get('running')) {
this._animation.stop();
=== modified file 'lib/lp/app/javascript/lp.js'
--- lib/lp/app/javascript/lp.js 2011-07-08 11:27:17 +0000
+++ lib/lp/app/javascript/lp.js 2011-07-08 11:27:23 +0000
@@ -85,37 +85,6 @@
});
}
-function toggleExpandableTableRow(element_id) {
- var row = document.getElementById(element_id);
- var view_icon = document.getElementById(element_id + "-arrow");
- if (row.style.display == "table-row") {
- row.style.display = "none";
- view_icon.setAttribute("src", "/@@/treeCollapsed");
- } else {
- row.style.display = "table-row";
- view_icon.setAttribute("src", "/@@/treeExpanded");
- }
- return false;
-}
-
-function toggleExpandableTableRows(class_name) {
- var view_icon = document.getElementById(class_name + "-arrow");
- var all_page_tags = document.getElementsByTagName("*");
- for (i = 0; i < all_page_tags.length; i++) {
- row = all_page_tags[i];
- if (row.className == class_name) {
- if (row.style.display == "table-row") {
- row.style.display = "none";
- view_icon.setAttribute("src", "/@@/treeCollapsed");
- } else {
- row.style.display = "table-row";
- view_icon.setAttribute("src", "/@@/treeExpanded");
- }
- }
- }
- return false;
-}
-
// Enable or disable the beta.launchpad.net redirect
function setBetaRedirect(enable) {
var expire = new Date();
=== modified file 'lib/lp/app/javascript/tests/test_expander.js'
--- lib/lp/app/javascript/tests/test_expander.js 2011-07-08 11:27:17 +0000
+++ lib/lp/app/javascript/tests/test_expander.js 2011-07-08 11:27:23 +0000
@@ -95,6 +95,14 @@
Y.Assert.areSame(animate, expander._animation.get('node'));
},
+ test_no_animate_node: function() {
+ // When config.no_animation is true, no animation
+ // is constructed or used.
+ var expander = this.makeExpander(
+ undefined, { config: { no_animation: true } });
+ Y.Assert.isUndefined(expander._animation);
+ },
+
test_loaded_is_true_if_no_loader_is_defined: function() {
var icon = Y.Node.create('<p></p>'),
content = Y.Node.create('<p></p>');
=== modified file 'lib/lp/bugs/javascript/filebug_dupefinder.js'
--- lib/lp/bugs/javascript/filebug_dupefinder.js 2011-05-04 16:25:30 +0000
+++ lib/lp/bugs/javascript/filebug_dupefinder.js 2011-07-08 11:27:23 +0000
@@ -77,19 +77,6 @@
}
/**
- * Collapse the details for a bug and set its expander arrow to
- * 'collapsed'
- * @param expander The expander to collapse.
- */
-function collapse_bug_details(expander) {
- var bug_details_div = get_details_div(expander);
- var anim = Y.lazr.effects.slide_in(bug_details_div);
- anim.run();
-
- expander.set(SRC, EXPANDER_COLLAPSED);
-}
-
-/**
* Show the bug reporting form and collapse all bug details forms.
*/
function show_bug_reporting_form() {
@@ -198,7 +185,7 @@
duplicate_div.set(INNER_HTML, response.responseText);
bug_already_reported_expanders = Y.all(
- 'img.bug-already-reported-expander');
+ 'td.bug-already-reported-expander');
if (bug_already_reported_expanders.size() > 0) {
// If there are duplicates shown, set up the JavaScript of
// the duplicates that have been returned.
@@ -370,31 +357,19 @@
namespace.setup_dupes = function() {
bug_already_reported_expanders = Y.all(
- 'img.bug-already-reported-expander');
+ 'td.bug-already-reported-expander');
var bug_reporting_form = Y.one('#bug-reporting-form');
if (bug_already_reported_expanders.size() > 0) {
- // Collapse all the details divs, since we don't want them
- // expanded first up.
- Y.each(Y.all('div.duplicate-details'), function(div) {
- collapse_bug_details(div);
- });
-
// Set up the onclick handlers for the expanders.
Y.each(Y.all('.similar-bug'), function(row) {
var bug_details_div = row.one('div.duplicate-details');
- var image = row.one('img.bug-already-reported-expander');
var bug_title_link = row.one('.duplicate-bug-link');
+ bug_title_link.addClass('js-action');
var view_bug_link = row.one('.view-bug-link');
-
- // Shut down the default action for the link and mark it
- // as a JS'd link. We do this as it's simpler than
- // trying to find all the bits of the row that we want
- // to make clickable.
- bug_title_link.addClass('js-action');
- bug_title_link.on('click', function(e) {
- e.preventDefault();
- });
+ var expander = new Y.lp.app.widgets.expander.Expander(
+ row, bug_details_div);
+ expander.setUp();
// The "view this bug" link shouldn't trigger the
// collapsible, so we stop the event from propagating.
@@ -414,10 +389,7 @@
// tabbing will expand the different bugs.
bug_title_link.on('focus', function(e) {
if (!bug_details_div.hasClass('lazr-opened')) {
- var anim = Y.lazr.effects.slide_out(bug_details_div);
- anim.run();
-
- image.set(SRC, EXPANDER_EXPANDED);
+ expander.render(true);
// If the bug reporting form is shown, hide it.
if (bug_reporting_form.getStyle(DISPLAY) == BLOCK) {
@@ -425,17 +397,6 @@
}
}
});
-
- row.on('click', function(e) {
- if (bug_details_div.hasClass('lazr-opened')) {
- collapse_bug_details(image);
- } else {
- var anim = Y.lazr.effects.slide_out(bug_details_div);
- anim.run();
-
- image.set(SRC, EXPANDER_EXPANDED);
- }
- });
});
// Hide the bug reporting form.
@@ -503,4 +464,4 @@
}, "0.1", {"requires": [
"base", "io", "oop", "node", "event", "json", "lazr.formoverlay",
- "lazr.effects"]});
+ "lazr.effects", "lp.app.widgets.expander"]});
=== modified file 'lib/lp/bugs/javascript/tests/test_filebug_dupfinder.js'
--- lib/lp/bugs/javascript/tests/test_filebug_dupfinder.js 2011-06-08 16:10:24 +0000
+++ lib/lp/bugs/javascript/tests/test_filebug_dupfinder.js 2011-07-08 11:27:23 +0000
@@ -142,12 +142,11 @@
search_text.set('value', 'foo');
var search_button = Y.one(Y.DOM.byId('field.actions.search'));
this.config.yio.io.responseText = ([
- '<img id="bug-details-expander" ',
- 'class="bug-already-reported-expander" ',
- 'src="/@@/treeCollapsed">',
+ '<table><tr><td id="bug-details-expander" ',
+ 'class="bug-already-reported-expander"></td></tr></table>',
'<input type="button" value="No, I need to report a new bug"',
' name="field.bug_already_reported_as"',
- ' id="bug-not-already-reported" style="display: block">'
+ ' id="bug-not-already-reported" style="display: block">',
].join(''));
this.config.yio.io.doAfter = function() {
// filebug container should not be visible when there are dups
=== modified file 'lib/lp/bugs/templates/bugtarget-macros-filebug.pt'
--- lib/lp/bugs/templates/bugtarget-macros-filebug.pt 2011-07-08 11:27:17 +0000
+++ lib/lp/bugs/templates/bugtarget-macros-filebug.pt 2011-07-08 11:27:23 +0000
@@ -346,13 +346,7 @@
<table tal:define="bugtask python:view.getRelevantBugTask(bug)">
<tbody>
<tr>
- <td>
- <img width="14" height="14" src="/@@/treeCollapsed"
- class="bug-already-reported-expander"
- tal:attributes="id string:bug-details-expander-bug-${bug/id};"
- />
- </td>
- <td>
+ <td class="bug-already-reported-expander">
<label tal:attributes="for string:bug-already-reported-as-${bug/id};
class bugtask/image:sprite_css"
tal:condition="bugtask"/>
=== modified file 'lib/lp/soyuz/browser/publishing.py'
--- lib/lp/soyuz/browser/publishing.py 2011-03-03 02:17:12 +0000
+++ lib/lp/soyuz/browser/publishing.py 2011-07-08 11:27:23 +0000
@@ -172,23 +172,6 @@
return self.context.dateremoved is not None
@property
- def js_connector(self):
- """Return the javascript glue for expandable rows mechanism."""
- return """
- <script type="text/javascript">
- registerLaunchpadFunction(function() {
- // Set the style of the expander icon so that it appears
- // clickable when js is enabled:
- var view_icon = document.getElementById('pub%s-expander');
- view_icon.style.cursor = 'pointer';
- connect('pub%s-expander', 'onclick', function (e) {
- toggleExpandableTableRow('pub%s');
- });
- });
- </script>
- """ % (self.context.id, self.context.id, self.context.id)
-
- @property
def removal_comment(self):
"""Return the removal comment or 'None provided'."""
removal_comment = self.context.removal_comment
=== modified file 'lib/lp/soyuz/templates/distributionsourcepackage-publishinghistory.pt'
--- lib/lp/soyuz/templates/distributionsourcepackage-publishinghistory.pt 2009-09-22 13:11:13 +0000
+++ lib/lp/soyuz/templates/distributionsourcepackage-publishinghistory.pt 2011-07-08 11:27:23 +0000
@@ -9,6 +9,24 @@
<body>
+<div metal:fill-slot="heading">
+ <script type="text/javascript">
+ LPS.use('node', 'lp.app.widgets.expander', function(Y) {
+ Y.on('domready', function() {
+ var all_expanders = Y.all('.expander-icon');
+ all_expanders.each(function(icon) {
+ var base_id = icon.get('id').replace('-expander', '');
+ var content_node = Y.one('#' + base_id);
+ var animate_node = content_node.one('ul');
+ var expander = new Y.lp.app.widgets.expander.Expander(
+ icon, content_node, { animate_node: animate_node });
+ expander.setUp();
+ });
+ });
+ });
+ </script>
+</div>
+
<div metal:fill-slot="main">
<div class="top-portlet">
=== modified file 'lib/lp/soyuz/templates/distroseries-queue.pt'
--- lib/lp/soyuz/templates/distroseries-queue.pt 2011-06-21 05:30:19 +0000
+++ lib/lp/soyuz/templates/distroseries-queue.pt 2011-07-08 11:27:23 +0000
@@ -9,6 +9,24 @@
<body>
+<div metal:fill-slot="heading">
+ <script type="text/javascript">
+ LPS.use('node', 'lp.app.widgets.expander', function(Y) {
+ Y.on('domready', function() {
+ var all_expanders = Y.all('.expander-link');
+ all_expanders.each(function(link) {
+ var base_id = link.get('id').replace('-icon', '');
+ var content = Y.one('tbody.' + base_id);
+ var content_rows = content.all('tr td *');
+ var expander = new Y.lp.app.widgets.expander.Expander(
+ link, content, { no_animation: true });
+ expander.setUp(true);
+ });
+ });
+ });
+ </script>
+</div>
+
<div metal:fill-slot="main"
tal:define="setup view/setupQueueList;
message view/performQueueAction;
@@ -78,12 +96,8 @@
other browsers look odd, now. :/
</tal:comment>
<td width="14" style="padding-top: 5px">
- <a tal:attributes="href string:#;
- onclick string:return toggleExpandableTableRows('${filelist_class}')">
- <img tal:attributes="id string:${filelist_class}-arrow"
- width="14" height="14"
- src="/@@/treeCollapsed" alt="view files"/>
- </a>
+ <span tal:attributes="id string:${filelist_class}-icon"
+ class="expander-link"> </span>
</td>
<td class="icon left" tal:condition="view/availableActions">
<input type="checkbox" name="QUEUE_ID"
@@ -128,7 +142,9 @@
</span>
</td>
</tr>
- <metal:filelist use-macro="template/macros/package-filelist"/>
+ <tbody tal:attributes="class filelist_class">
+ <metal:filelist use-macro="template/macros/package-filelist"/>
+ </tbody>
</tal:block>
</tal:batch>
<tr tal:condition="view/availableActions">
@@ -203,10 +219,8 @@
</tal:comment>
<tal:copy condition="packageupload/pending_delayed_copy">
- <tr tal:attributes="class string:${filelist_class}"
- tal:define="archive
- packageupload/sourcepackagerelease/upload_archive"
- style="display:none">
+ <tr tal:define="archive
+ packageupload/sourcepackagerelease/upload_archive">
<td />
<td tal:condition="view/availableActions" />
<td>Copied from
@@ -224,9 +238,7 @@
</tal:copy>
<tal:upload condition="not: packageupload/pending_delayed_copy">
- <tr tal:repeat="file packageupload/source_files"
- tal:attributes="class string:${filelist_class}"
- style="display:none">
+ <tr tal:repeat="file packageupload/source_files">
<td/>
<td tal:condition="view/availableActions"/>
<td
@@ -243,9 +255,7 @@
component_name package/component/name/lower;
section_name package/section/name/lower;
version package/version">
- <tr tal:repeat="file python:packageupload.binary_packages[package]"
- tal:attributes="class string:${filelist_class}"
- style="display:none">
+ <tr tal:repeat="file python:packageupload.binary_packages[package]">
<td/>
<td tal:condition="view/availableActions"/>
<td tal:define="libraryfilealias file/libraryfile">
@@ -262,9 +272,7 @@
</tal:package>
<tal:custom define="customfiles packageupload/customfiles">
- <tr tal:repeat="custom python:list(customfiles)"
- tal:attributes="class string:${filelist_class}"
- style="display:none">
+ <tr tal:repeat="custom python:list(customfiles)">
<td/>
<td tal:condition="view/availableActions"></td>
<td tal:define="libraryfilealias custom/libraryfilealias">
@@ -276,9 +284,7 @@
<tal:diffs condition="packageupload/sourcepackagerelease">
<tr tal:define="diffs packageupload/sourcepackagerelease/package_diffs"
- tal:repeat="diff diffs"
- tal:attributes="class string:${filelist_class}"
- style="display:none">
+ tal:repeat="diff diffs">
<td/>
<td tal:condition="view/availableActions"></td>
<td>
=== modified file 'lib/lp/soyuz/templates/publishinghistory-macros.pt'
--- lib/lp/soyuz/templates/publishinghistory-macros.pt 2009-07-17 17:59:07 +0000
+++ lib/lp/soyuz/templates/publishinghistory-macros.pt 2011-07-08 11:27:23 +0000
@@ -10,10 +10,9 @@
:version_url: The version link
</tal:comment>
<tr>
- <td style="white-space: nowrap"><span
- tal:attributes="id string:pub${context/id}-expander"
- ><img src="/@@/treeCollapsed" alt=""
- tal:attributes="id string:pub${context/id}-arrow" /></span>
+ <td style="white-space: nowrap">
+ <span tal:attributes="id string:pub${context/id}-expander"
+ class="expander-icon"> </span>
</td>
<td tal:content="view/date_last_changed/fmt:datetime"
>2005-08-24</td>
@@ -31,11 +30,11 @@
>2.0.39</a>
</td>
</tr>
- <tr tal:attributes="id string:pub${context/id}" style="display: none">
+ <tr tal:attributes="id string:pub${context/id}">
<td colspan="8">
<tal:block replace="structure context/@@+record-details" />
</td>
</tr>
- <tal:js_glue replace="structure view/js_connector" />
+
</metal:header>
