Merge lp:~cjohnston/launchpad/ic-view into lp:launchpad

[Chris Johnston]

Improve styling to inline comments. Props to daker for the CSS help.

Props to daker for helping out with the CSS. Also includes a fix from cprov to remove left over code when cancelling an editing function.

[Adnane Belmadiaf (daker)]

+1 from me

[Celso Providelo (cprov)]

Looks great, Chris!

I am not particularly sure about the font & line-height changes at this point, because they make inline comments too sparse. But that's more a personal taste issue than a opposition to land the changes.

Soon we will get some time with UI experts and they will guide the final tweaks to get this released.

Thanks for working on this.

review: Approve

=== 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-09 19:59:19 +0000
@@ -204,3 +204,52 @@
   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: .5em 12px;
+  font-family: Ubuntu,"Bitstream Vera Sans","DejaVu Sans",Tahoma,sans-serif;
+  font-size: 12px;
+}
+td.inline-comment {
+  border: 1px solid rgb(221, 221, 221);
+  border-right: 0;
+  margin: 0 0 20px 0;
+  line-height: 25px;
+  background-color: rgb(248, 248, 255);
+  font-family: Ubuntu,"Bitstream Vera Sans","DejaVu Sans",Tahoma,sans-serif;
+  font-size: 12px;
+  padding: 0 12px 0;
+  word-break: break-word;
+}
+
+.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: -6px!important;
+  overflow: hidden;
+}
+.inline-comment .yui3-ieditor-content {top: 5px; left: -5px;}
+.inline-comment .yui3-ieditor-input textarea {
+  width: 92%;
+  max-width: 92%;
+  border-top: 1px solid rgb(214, 214, 214);
+  float: left;
+}
+.inline-comment .yui3-ieditor-input .bg-top-label { margin-right: 10px; }
+.inline-comment .bg-top-label {
+  margin-right: -7px!important;
+  background-image: none!important;
+  margin-top: 18px!important;
+}
+tr.ict-content .yui3-ieditor-multiline .yui3-ieditor-btns .bg-top-label button.lazr-btn { text-indent: -9999px; }
+.inline-comment .bg-top-label button.lazr-btn { text-indent: -9999px; }
+.diff-content .boardCommentBody { padding: 0!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-09 19:59:19 +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-09 19:59:19 +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() {