launchpad-reviewers team mailing list archive

[Chris Johnston <chris.johnston@xxxxxxxxxxxxx>]

Chris Johnston has proposed merging lp:~cjohnston/launchpad/ic-view into lp:launchpad.

Commit message:
Improve styling to inline comments

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjohnston/launchpad/ic-view/+merge/214851

Props to daker for helping out with the CSS
-- 
https://code.launchpad.net/~cjohnston/launchpad/ic-view/+merge/214851
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjohnston/launchpad/ic-view into lp:launchpad.

=== modified file 'lib/canonical/launchpad/icing/style.css'
--- lib/canonical/launchpad/icing/style.css	2013-09-05 03:57:28 +0000
+++ lib/canonical/launchpad/icing/style.css	2014-04-08 23:09:03 +0000
@@ -237,7 +237,7 @@
     width:2%;
     border-left: 1px solid #ddd;
 }
-.boardCommentBody {padding: 0.5em 12px 0;}
+#conversation .boardCommentBody { padding: .5em 12px 0; }
 .boardBugActivityBody {
   padding: 0.5em 12px;
   background-color: #fafafa;
@@ -585,14 +585,16 @@
    white-space: pre-wrap;
    width: 100%;
    background-color: white;
-   margin-bottom: 0.5em;
-}
-table.diff .text { padding-left: 0.5em; }
+}
+table.diff .text {
+  padding-left: 0.5em;
+  border-left: 1px solid rgb(221, 221, 221);
+}
 table.diff .line-no {
   text-align: right;
   background-color: #f6f6f6;
   color: #999;
-  border-right: solid 4px white;
+  padding-right: 4px;
 }
 table.diff .diff-chunk, table.diff .diff-file, table.diff .diff-header {
  font-weight: bold;

=== modified file 'lib/lp/app/javascript/inlineedit/assets/skins/sam/editor-skin.css'
--- lib/lp/app/javascript/inlineedit/assets/skins/sam/editor-skin.css	2012-08-23 19:56:15 +0000
+++ lib/lp/app/javascript/inlineedit/assets/skins/sam/editor-skin.css	2014-04-08 23:09:03 +0000
@@ -204,3 +204,39 @@
   background: url('edit.png') top left no-repeat;
   padding: 2px 0 0 18px;
 }
+
+tr.ict-header {
+  margin: 20px 0 0 0;
+}
+td.ic-header {
+  border: 1px solid rgb(221, 221, 221);
+  border-right: 0;
+  padding-left: 5px;
+}
+td.inline-comment {
+  border: 1px solid rgb(221, 221, 221);
+  border-right: 0;
+  margin: 0 0 20px 0;
+  padding-left: 5px;
+  line-height: 25px;
+  color: rgb(119, 119, 119);
+  background-color: rgb(248, 248, 255);
+}
+
+.inline-comment>div { line-height: 25px; }
+.inline-comment .yui3-ieditor { padding-right:0 !important }
+.inline-comment .yui3-ieditor > yui3-ieditor-input { width: auto!important }
+.inline-comment .yui3-ieditor-input {
+  width: auto!important;
+  margin-right: 0;
+  margin-bottom: 5px;
+  margin-top: -35px!important;
+}
+.inline-comment .yui3-ieditor-content {top: 5px; left: -5px;}
+.inline-comment .yui3-ieditor-input textarea {
+  width: 100%;
+  max-width: 100%;
+  border-top: 1px solid rgb(214, 214, 214);
+}
+.inline-comment .yui3-ieditor-input .bg-top-label { margin-right: 10px; }
+.inline-comment .bg-top-label { margin-right: -7px!important; }

=== modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-04-04 05:23:48 +0000
+++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-04-08 23:09:03 +0000
@@ -50,6 +50,7 @@
                 handling_request = false;
                 if (namespace.inlinecomments[line_number] === undefined) {
                     header_row.remove(true);
+                    content_row.remove(true);
                 }
                 return;
             }
@@ -83,8 +84,8 @@
 
 namespace.create_row = function(comment) {
     var ident = comment.line_number + '-draft';
-    var headerrow = Y.Node.create(
-        '<tr><td colspan="2"></td></tr>').addClass('ict-header');
+    var headerrow = Y.Node.create('<tr><td class="line-no"></td>' +
+        '<td class="ic-header"></td></tr>').addClass('ict-header');
     var headerspan;
     var newrow;
     if (Y.Lang.isUndefined(comment.person)) {
@@ -92,8 +93,9 @@
         headerspan = Y.Node.create('<span></span>').set(
             'text', 'Draft comment.');
         headerrow.set('id', 'ict-' + ident + '-header');
-        newrow = Y.Node.create('<tr><td></td><td><div>' +
-            '<span class="yui3-editable_text-text"></span>' +
+        newrow = Y.Node.create('<tr><td class="line-no"></td>' +
+            '<td class="inline-comment"><div>' +
+            '<span class="yui3-editable_text-text inline-comment"></span>' +
             '<div class="yui3-editable_text-trigger"></div>' +
             '</div></td></tr>').set('id', 'ict-' + ident);
         newrow.one('td>div').set('id', 'inlinecomment-' + ident);
@@ -123,10 +125,12 @@
         var fmt_date = 'on ' + Y.Date.format(
             new Date(comment.date), {format: '%Y-%m-%d'});
         headerspan.one('span').set('text', fmt_date);
-        newrow = Y.Node.create('<tr><td></td><td><span></span></td></tr>');
+        newrow = Y.Node.create('<tr><td class="line-no"></td>' +
+            '<td class="inline-comment"><span></span></td></tr>');
         newrow.one('span').set('text', comment.text);
     }
-    headerrow.one('td').appendChild(headerspan);
+    var lineno = headerrow.one('td')
+    lineno.get('nextSibling').appendChild(headerspan);
 
     // We want to have the comments in order after the line.
     var tr = Y.one('#diff-line-' + (parseInt(comment.line_number, 10) + 1));

=== modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
--- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-04-04 05:23:48 +0000
+++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-04-08 23:09:03 +0000
@@ -145,6 +145,18 @@
             Y.Assert.areEqual('diff-line-2', line.next().get('id'));
         },
 
+        test_edit_cancelling: function() {
+            // Cancelling a inline comment attempt on a row with
+            // no comments does not leave an empty row behind.
+            module.add_doubleclick_handler();
+            var line  = Y.one('#diff-line-1');
+            line.simulate('dblclick');
+            ic_area = line.next().next();
+            ic_area.one('.yui3-ieditor-input>textarea').set('value', '');
+            ic_area.one('.lazr-neg').simulate('click');
+            Y.Assert.areEqual('diff-line-2', line.next().get('id'));
+        },
+
         test_feature_flag_off: function() {
             var called = false;
             add_doubleclick_handler = function() {