launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10193
[Merge] lp:~jcsackett/launchpad/comment-list-delegates into lp:launchpad
j.c.sackett has proposed merging lp:~jcsackett/launchpad/comment-list-delegates into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1020652 in Launchpad itself: "Javascript comment code is out of date"
https://bugs.launchpad.net/launchpad/+bug/1020652
For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/comment-list-delegates/+merge/116376
Summary
=======
There's no reason that comment hiding shouldn't be part of the comment code in
our javascript module. We have a nice comment object that controls comment
stuff--that should do the hide work.
Preimp
======
Spoke with Rick Harding about setting up the use of delegate.
Implementation
==============
The hide comment methods are now part of the comment widget. This isn't
perfect, b/c the comment widget is more of a comment form widget, but also
controls a bunch of comment display stuff, making it a suitable place to merge
this code.
There's a strong case that a subsequent branch should come along and rename
things to something more true to case, but as that may also require
determining if the comment stuff is indeed a *widget* and more involved
refactor, it's out of scope for this branch.
Tests
=====
bin/test -vvct comment --layer=YUI
QA
==
Make sure comment hiding works as expected on bug comments.
LoC
===
Cleaning up comments is part of cleanup for disclosure.
Lint
====
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/tests/test_hide_comment.js
lib/lp/app/javascript/tests/test_hide_comment.html
lib/lp/bugs/templates/bugtask-index.pt
lib/lp/app/javascript/comment.js
--
https://code.launchpad.net/~jcsackett/launchpad/comment-list-delegates/+merge/116376
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/comment-list-delegates into lp:launchpad.
=== modified file 'lib/lp/app/javascript/comment.js'
--- lib/lp/app/javascript/comment.js 2012-07-16 14:32:30 +0000
+++ lib/lp/app/javascript/comment.js 2012-07-23 21:14:25 +0000
@@ -2,6 +2,9 @@
var namespace = Y.namespace('lp.app.comment');
+
+
+
namespace.Comment = Y.Base.create('comment', Y.Widget, [], {
/**
* Initialize the Comment
@@ -23,6 +26,15 @@
},
/**
+ * Destructor
+ *
+ * @method destructor
+ */
+ destructor: function() {
+ var comment_list_container = Y.one('#maincontentstub');
+ comment_list_container.detachAll();
+ },
+ /**
* Return the Submit button.
*
* This is provided so that it can be overridden in subclasses.
@@ -169,6 +181,73 @@
this.submit_button, this.progress_message);
this.set_disabled(false);
},
+
+ /**
+ * Update a comment appearance on hide commands.
+ *
+ * @method update_comment
+ */
+ update_comment: function (link, comment) {
+ var text = link.get('text').trim();
+ if (text === this.get('hide_text')) {
+ comment.removeClass(this.get('hidden_class'));
+ link.set('text', this.get('unhide_text'));
+ } else {
+ comment.addClass(this.get('hidden_class'));
+ link.set('text', this.get('hide_text'));
+ }
+ },
+
+ /**
+ * Set the comment visibility.
+ *
+ * @method set_visibility
+ */
+ set_visibility: function (parameters, callbacks) {
+ // comment_context must be setup on pages using this js, and is the
+ // context for a comment (e.g. bug).
+ var comment_context = LP.cache.comment_context;
+ var lp_client = new Y.lp.client.Launchpad();
+ var config = {
+ on: {
+ success: callbacks.success,
+ failure: callbacks.failure
+ },
+ parameters: parameters
+ };
+ lp_client.named_post(
+ comment_context.self_link, 'setCommentVisibility', config);
+ },
+
+ /**
+ * toggle the hidden status of the comment.
+ *
+ * @method toggle_hidden
+ */
+ toggle_hidden: function (link) {
+ var comment = link.get('parentNode').get('parentNode');
+ var visible = comment.hasClass('adminHiddenComment');
+ var comment_number = parseInt(
+ link.get('id').replace('mark-spam-', ''), 10);
+ parameters = {
+ visible: visible,
+ comment_number: comment_number
+ };
+ var that = this;
+ this.set_visibility(parameters, {
+ // We use red flash on failure so admins know it didn't work.
+ // There's no green flash on success, b/c the change in bg
+ // color provides an immediate visual cue.
+ success: function () {
+ that.update_comment(link, comment);
+ comment.toggleClass(that.get('hidden_class'));
+ },
+ failure: function () {
+ Y.lp.anim.red_flash({node:comment});
+ }
+ });
+ },
+
/**
* Implementation of Widget.bindUI: Bind events to methods.
*
@@ -182,6 +261,13 @@
this.comment_input.on('keyup', this.syncUI, this);
this.comment_input.on('mouseup', this.syncUI, this);
this.submit_button.on('click', this.add_comment, this);
+ var comment_list_container = Y.one('#maincontentstub');
+ var that = this;
+ var callback = function(e) {
+ e.halt();
+ that.toggle_hidden(this);
+ };
+ comment_list_container.delegate('click', callback, 'a.mark-spam');
},
/**
* Implementation of Widget.syncUI: Update appearance according to state.
@@ -198,7 +284,10 @@
lp_client: {
valueFn: function() { return new Y.lp.client.Launchpad() }
},
- animate: { value: true }
+ animate: { value: true },
+ hidden_class: { value: "adminHiddenComment" },
+ hide_text: { value: "Hide comment" },
+ unhide_text: { value: "Unhide comment" }
},
});
=== removed file 'lib/lp/app/javascript/hide_comment.js'
--- lib/lp/app/javascript/hide_comment.js 2012-03-14 12:51:18 +0000
+++ lib/lp/app/javascript/hide_comment.js 1970-01-01 00:00:00 +0000
@@ -1,72 +0,0 @@
-/* Copyright 2011 Canonical Ltd. This software is licensed under the
- * GNU Affero General Public License version 3 (see the file LICENSE).
- *
- * @namespace Y.lp.comments.hide
- * @requires dom, node, lp.anim, lp.client
- */
-YUI.add('lp.comments.hide', function(Y) {
-var namespace = Y.namespace('lp.comments.hide');
-
-var hidden_class = "adminHiddenComment";
-var hide_text = "Hide comment";
-var unhide_text = "Unhide comment";
-
-function update_comment(link, comment) {
- var text = link.get('text').trim();
- if (text === hide_text) {
- comment.removeClass(hidden_class);
- link.set('text', unhide_text);
- } else {
- comment.addClass(hidden_class);
- link.set('text', hide_text);
- }
-}
-
-function set_visibility(parameters, callbacks) {
- // comment_context must be setup on pages using this js, and is the
- // context for a comment (e.g. bug).
- var comment_context = LP.cache.comment_context;
- var lp_client = new Y.lp.client.Launchpad();
- var config = {
- on: {
- success: callbacks.success,
- failure: callbacks.failure
- },
- parameters: parameters
- };
- lp_client.named_post(
- comment_context.self_link, 'setCommentVisibility', config);
-}
-
-function toggle_hidden(link) {
- var comment = link.get('parentNode').get('parentNode');
- var visible = comment.hasClass('adminHiddenComment');
- var comment_number = parseInt(
- link.get('id').replace('mark-spam-', ''), 10);
- parameters = {
- visible: visible,
- comment_number: comment_number
- };
- set_visibility(parameters, {
- // We use red flash on failure so admins know it didn't work.
- // There's no green flash on success, b/c the change in bg
- // color provides an immediate visual cue.
- success: function () {
- update_comment(link, comment);
- comment.toggleClass(hidden_class);
- },
- failure: function () {
- Y.lp.anim.red_flash({node:comment});
- }
- });
-}
-namespace.toggle_hidden = toggle_hidden;
-
-function setup_hide_controls() {
- Y.on('click', function(e) {
- e.halt();
- namespace.toggle_hidden(this);
- }, '.mark-spam');
-}
-namespace.setup_hide_controls = setup_hide_controls;
-}, "0.1", {"requires": ["dom", "node", "lp.anim", "lp.client"]});
=== modified file 'lib/lp/app/javascript/tests/test_hide_comment.html'
--- lib/lp/app/javascript/tests/test_hide_comment.html 2012-03-14 04:41:36 +0000
+++ lib/lp/app/javascript/tests/test_hide_comment.html 2012-07-23 21:14:25 +0000
@@ -31,7 +31,7 @@
src="../../../../../build/js/lp/app/lp.js"></script>
<!-- The module under test. -->
- <script type="text/javascript" src="../hide_comment.js"></script>
+ <script type="text/javascript" src="../comment.js"></script>
<!-- Placeholder for any css asset for this module. -->
<!-- <link rel="stylesheet" href="../assets/hide_comment-core.css" /> -->
@@ -42,27 +42,42 @@
</head>
<body class="yui3-skin-sam">
<ul id="suites">
- <!-- <li>lp.large_indicator.test</li> -->
- <li>lp.hide_comment.test</li>
+ <li>lp.app.comment.hide.test</li>
</ul>
<!-- The example markup required by the script to run -->
- <div class="boardComment">
- <div class="boardCommentDetails">Details</div>
- <div class="boardCommentBody"><p>Foo bar baz.</p></div>
- <div class="boardCommentFooter">
- <a id="mark-spam-0" class="js-action sprite edit" href="#"
- >Hide comment</a>
- </div>
- </div>
+ <div id="maincontentstub">
+ <div class="boardComment">
+ <div class="boardCommentDetails">Details</div>
+ <div class="boardCommentBody"><p>Foo bar baz.</p></div>
+ <div class="boardCommentFooter">
+ <a id="mark-spam-0" class="mark-spam js-action sprite edit"
+ href="#">Hide comment</a>
+ </div>
+ </div>
- <div id="hidden-comment" class="boardComment adminHiddenComment">
- <div class="boardCommentDetails">Details</div>
- <div class="boardCommentBody"><p>Click here for a diploma!</p></div>
- <div class="boardCommentFooter">
- <a id="mark-spam-1" class="js-action sprite edit" href="#">
- Unhide comment
- </a>
- </div>
+ <div id="hidden-comment" class="boardComment adminHiddenComment">
+ <div class="boardCommentDetails">Details</div>
+ <div class="boardCommentBody"><p>Click here for a diploma!</p></div>
+ <div class="boardCommentFooter">
+ <a id="mark-spam-1" class="mark-spam js-action sprite edit"
+ href="#">Unhide comment
+ </a>
+ </div>
+ </div>
+ <div id="add-comment-form">
+ <h2>Add comment</h2>
+ <form action="+addcomment" method="post"
+ enctype="multipart/form-data" accept-charset="UTF-8">
+ <textarea cols="60"
+ id="field.comment" name="field.comment" rows="15"></textarea>
+ <div class="actions">
+ <input disabled="disabled"
+ style="display: inline;" id="field.actions.save"
+ name="field.actions.save" value="Post Comment"
+ class="button js-action" type="submit"/>
+ </div>
+ </form>
+ </div>
</div>
</body>
</html>
=== modified file 'lib/lp/app/javascript/tests/test_hide_comment.js'
--- lib/lp/app/javascript/tests/test_hide_comment.js 2011-07-08 05:12:39 +0000
+++ lib/lp/app/javascript/tests/test_hide_comment.js 2012-07-23 21:14:25 +0000
@@ -1,14 +1,13 @@
-/* Copyright 2011 Canonical Ltd. This software is licensed under the
+/* Copyright 2011-2012 Canonical Ltd. This software is licensed under the
* GNU Affero General Public License version 3 (see the file LICENSE).
*/
-YUI().use('lp.testing.runner', 'test', 'console', 'node',
- 'lp.comments.hide', 'node-event-simulate', function(Y) {
-
- var suite = new Y.Test.Suite("lp.comments.hide Tests");
-
- suite.add(new Y.Test.Case({
- name: 'hide_comments',
+YUI.add('lp.app.comment.hide.test', function (Y) {
+
+ var tests = Y.namespace('lp.app.comment.hide.test');
+ tests.suite = new Y.Test.Suite("lp.comments.hide Tests");
+ tests.suite.add(new Y.Test.Case({
+ name: 'lp.app.comments.hide_test',
setUp: function() {
// Monkeypatch LP to avoid network traffic and to allow
@@ -32,13 +31,19 @@
LP.cache.comment_context = {
self_link: 'https://launchpad.dev/api/devel/some/comment/'
};
+ this.comment = new Y.lp.app.comment.Comment();
+ this.comment.render();
+ },
+
+ tearDown: function() {
+ this.comment.destroy();
},
test_hide: function () {
- link = Y.one('#mark-spam-0');
- comment = Y.one('.boardComment');
- Y.lp.comments.hide.toggle_hidden(link);
- Y.Assert.isTrue(comment.hasClass('adminHiddenComment'));
+ var link = Y.one('#mark-spam-0');
+ var comment_node = Y.one('.boardComment');
+ link.simulate('click');
+ Y.Assert.isTrue(comment_node.hasClass('adminHiddenComment'));
Y.Assert.areEqual('Unhide comment', link.get('text'),
'Link text should be \'Unhide comment\'');
Y.Assert.areEqual(
@@ -52,13 +57,13 @@
Y.Assert.areEqual(
0, LP.cache.call_data.called_config.parameters.comment_number,
'Called with wrong wrong comment number.');
- },
+ },
test_unhide: function () {
- link = Y.one('#mark-spam-1');
- comment = Y.one('#hidden-comment');
- Y.lp.comments.hide.toggle_hidden(link);
- Y.Assert.isFalse(comment.hasClass('adminHiddenComment'));
+ var link = Y.one('#mark-spam-1');
+ var comment_node = Y.one('#hidden-comment');
+ link.simulate('click');
+ Y.Assert.isFalse(comment_node.hasClass('adminHiddenComment'));
Y.Assert.areEqual('Hide comment', link.get('text'),
'Link text should be \'Hide comment\'');
Y.Assert.areEqual(
@@ -75,7 +80,5 @@
}
}));
- Y.lp.testing.Runner.run(suite);
-
-});
-
+}, '0.1', {'requires': ['test', 'lp.testing.helpers', 'console',
+ 'node-event-simulate', 'lp.app.comment']});
=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt 2012-06-28 16:00:11 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt 2012-07-23 21:14:25 +0000
@@ -11,15 +11,13 @@
tal:content="view/available_official_tags_js" />
<script type="text/javascript">
LPJS.use('base', 'node', 'oop', 'event', 'lp.bugs.bugtask_index',
- 'lp.bugs.subscribers',
- 'lp.code.branchmergeproposal.diff', 'lp.comments.hide',
+ 'lp.bugs.subscribers', 'lp.code.branchmergeproposal.diff',
function(Y) {
Y.on('domready', function() {
Y.lp.code.branchmergeproposal.diff.connect_diff_links();
Y.lp.bugs.bugtask_index.setup_bugtask_index();
Y.lp.bugs.bugtask_index.setup_bugtask_table();
LP.cache.comment_context = LP.cache.bug;
- Y.lp.comments.hide.setup_hide_controls();
var sl = new Y.lp.bugs.subscribers.createBugSubscribersLoader({
container_box: '#other-bug-subscribers',
subscribers_details_view:
Follow ups
