launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #16764
Re: [Merge] lp:~wgrant/launchpad/ic-js-cleanup into lp:launchpad
Inline comments:
> --- lib/canonical/launchpad/icing/css/modifiers.css 2014-05-14 09:23:56 +0000
> +++ lib/canonical/launchpad/icing/css/modifiers.css 2014-05-19 09:28:43 +0000
> @@ -166,7 +166,8 @@
> }
>
> pre.changelog,
> -table.diff,
> +table.diff td.line-no,
> +table.diff td.text,
> .comment-text,
> .bug-activity {
> font-family: monospace;
>
> === modified file 'lib/canonical/launchpad/icing/style.css'
> --- lib/canonical/launchpad/icing/style.css 2014-05-14 09:23:56 +0000
> +++ lib/canonical/launchpad/icing/style.css 2014-05-19 09:28:43 +0000
> @@ -594,11 +594,16 @@
> color: #999;
> border-right: solid 4px white;
> }
> +table.diff .line-no.active {
> + background: url(/@@/edit) #f6f6f6 center left no-repeat;
> +}
> table.diff .diff-chunk, table.diff .diff-file, table.diff .diff-header {
> font-weight: bold;
> }
> table.diff .diff-added { background-color: #92ED92; }
> table.diff .diff-removed { background-color: #FF7F7F; }
> +table.diff .inline-comments > td > div { margin: 0px 1em; }
> +
> .back-link { margin-top: 3em; }
>
> .reviews {
>
> === modified file 'lib/lp/app/javascript/inlineedit/assets/skins/sam/editor-skin.css'
> --- lib/lp/app/javascript/inlineedit/assets/skins/sam/editor-skin.css 2014-05-12 19:03:15 +0000
> +++ lib/lp/app/javascript/inlineedit/assets/skins/sam/editor-skin.css 2014-05-19 09:28:43 +0000
> @@ -204,52 +204,3 @@
> 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-all;
> -}
ouch!
> -
> -.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-05-15 04:18:54 +0000
> +++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js 2014-05-19 09:28:43 +0000
> @@ -18,48 +18,41 @@
>
> namespace.lp_client = new Y.lp.client.Launchpad();
>
> +namespace.cleanup_empty_comment_containers = function() {
> + Y.one('.diff').all('tr.inline-comments > td > div:empty').each(
> + function(node) {
> + node.ancestor('tr.inline-comments').remove(true);
> + });
> +};
> +
> namespace.add_doubleclick_handler = function() {
> - var rows = Y.one('.diff').all('tr');
> var handling_request = false;
> handler = function(e) {
> if (handling_request === true) {
> return;
> }
> handling_request = true;
> - var linenumberdata = e.currentTarget.one('.line-no');
> - var line_number = linenumberdata.get('text');
> - // Retrieve or create a row container for the comment editor.
> - var header_row = Y.one(
> - '#ict-' + line_number + '-draft-header');
> - if (header_row === null) {
> - header_row = namespace.create_row(
> - {'line_number': line_number});
> + var text_row = e.currentTarget.ancestor('tr', true);
> + var line_number = text_row.one('.line-no').get('text');
> + // Retrieve or create a container for the comment editor.
> + var draft_div = Y.one('#comments-' + text_row.get('id') + ' .draft');
> + if (draft_div === null) {
> + draft_div = namespace.create_row({'line_number': line_number});
> }
> - var content_row = header_row.next();
> var widget = new Y.EditableText({
> - contentBox: content_row.one(
> - '#inlinecomment-' + line_number + '-draft'),
> + contentBox: draft_div.one('.boardCommentBody div'),
> initial_value_override: namespace.inlinecomments[line_number],
> accept_empty: true,
> multiline: true,
> buttons: 'top'
> });
> widget.render();
> - handle_widget_button = function(saved, comment) {
> - if (saved === false) {
> - handling_request = false;
> - if (namespace.inlinecomments[line_number] === undefined) {
> - header_row.remove(true);
> - content_row.remove(true);
> - }
> - return;
> - }
> -
> - namespace.inlinecomments[line_number] = comment;
> - if (comment === '') {
> + widget.editor.on('save', function() {
> + namespace.inlinecomments[line_number] = this.get('value');
> + if (this.get('value') === '') {
> delete namespace.inlinecomments[line_number];
> - header_row.remove(true);
> - content_row.remove(true);
> + draft_div.remove(true);
> + namespace.cleanup_empty_comment_containers();
> }
> var config = {
> on: {
> @@ -75,45 +68,67 @@
> namespace.lp_client.named_post(
> LP.cache.context.self_link, 'saveDraftInlineComment', config);
> handling_request = false;
> - };
> - widget.editor.on('save', function() {
> - handle_widget_button(true, this.get('value'));
> });
> widget.editor.on('cancel', function(e) {
> - handle_widget_button(false, this.get('value'));
> + handling_request = false;
> + // If there's no saved comment to return to, just remove the
> + // draft UI.
> + if (namespace.inlinecomments[line_number] === undefined) {
> + draft_div.remove(true);
> + namespace.cleanup_empty_comment_containers();
> + }
> });
> widget._triggerEdit(e);
> };
> - rows.on('dblclick', handler);
> +
> + // The editor can be invoked by double-clicking a diff line or
> + // clicking the line number. Hovering over a line shows an edit icon
> + // near the line number, to hint that it's clickable.
> + // XXX: Introduce a separate comment column.
> + Y.one('.diff').delegate('dblclick', handler, 'tr[id^="diff-line"]');
> + Y.one('.diff').delegate('click', handler, 'tr[id^="diff-line"] .line-no');
> + Y.one('.diff').delegate('hover',
> + function(e) {
> + e.currentTarget.one('.line-no').addClass('active');
> + },
> + function(e) {
> + e.currentTarget.one('.line-no').removeClass('active');
> + },
> + 'tr[id^="diff-line"]');
> };
>
> namespace.create_row = function(comment) {
> - var ident = comment.line_number + '-draft';
> - var headerrow = Y.Node.create(
> - '<tr><td class="line-no"></td><td class="ic-header"></td></tr>')
> - .addClass('ict-header');
> - var headerspan;
> - var newrow;
> + // Find or create the container for this line's comments.
> + var comments_tr = Y.one('#comments-diff-line-' + comment.line_number);
> + if (comments_tr === null) {
> + var comments_tr = Y.Node.create(
> + '<tr class="inline-comments">'
> + + '<td colspan="2"><div></div></td></tr>')
> + .set('id', 'comments-diff-line-' + comment.line_number);
> + Y.one('#diff-line-' + comment.line_number)
> + .insert(comments_tr, 'after');
> + }
> + comments_div = comments_tr.one('div');
> +
> + var newrow = Y.Node.create(
> + '<div class="boardComment">' +
> + '<div class="boardCommentDetails"></div>' +
> + '<div class="boardCommentBody"><div></div></div>' +
> + '</div>');
> if (Y.Lang.isUndefined(comment.person)) {
> // Creates a draft inline comment area.
> - headerspan = Y.Node.create('<span></span>').set(
> - 'text', 'Draft comment');
> - headerrow.set('id', 'ict-' + ident + '-header');
> - newrow = Y.Node.create(
> - '<tr><td class="line-no"></td>' +
> - '<td class="inline-comment"><div>' +
> - '<span class="yui3-editable_text-text"></span>' +
> - '<div class="yui3-editable_text-trigger"></div>' +
> - '</div></td></tr>').set('id', 'ict-' + ident);
> - newrow.one('td>div').set('id', 'inlinecomment-' + ident);
> newrow.addClass('draft');
> + newrow.one('.boardCommentDetails').set('text', 'Draft comment');
> + newrow.one('.boardCommentBody div')
> + .append('<span class="yui3-editable_text-text"></span>')
> + .append('<div class="yui3-editable_text-trigger"></div>');
> if (!Y.Lang.isUndefined(comment.text)) {
> namespace.inlinecomments[comment.line_number] = comment.text;
> newrow.one('span').set('text', comment.text);
> }
> } else {
> // Creates a published inline comment area.
> - headerspan = Y.Node.create(
> + var headerspan = Y.Node.create(
> '<span>Comment by <a></a> <span></span></span>');
> // Wrap resources coming from LP.cache, as they come from
> // the LP API (updates during the page life-span). This way
> @@ -133,42 +148,24 @@
> 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 class="line-no"></td>' +
> - '<td class="inline-comment"><span></span></td></tr>');
> - newrow.one('span').set('text', comment.text);
> + newrow.one('.boardCommentDetails').append(headerspan);
> + newrow.one('.boardCommentBody div').append('<span></span>');
> + newrow.one('.boardCommentBody').one('span').set('text', comment.text);
> }
> - headerrow.one('td:last-child').appendChild(headerspan);
>
> - // We want to have the comments in order after the line.
> - var base_line = Y.one(
> - '#diff-line-' + (parseInt(comment.line_number, 10) + 1));
> - if (base_line !== null) {
> - // If there are drafts, they should remain at the bottom.
> - if (base_line.previous().hasClass('draft')) {
> - base_line = base_line.previous().previous();
> - }
> - base_line.insert(headerrow, 'before');
> + // Ensure that drafts always end up at the end.
> + var maybe_draft = comments_div.one('.draft');
> + if (comment.person !== undefined && maybe_draft !== null) {
> + maybe_draft.insert(newrow, 'before');
> } else {
> - // If this is the last row, grab the last child.
> - base_line = Y.one('.diff>tbody>tr:last-child');
> - // If there are drafts, they should remain at the bottom.
> - if (base_line.hasClass('draft')) {
> - base_line = base_line.previous().previous();
> - }
> - base_line.insert(headerrow, 'after');
> + comments_div.appendChild(newrow);
> }
> - headerrow.insert(newrow, 'after');
> -
> - return headerrow;
> + return newrow;
> };
>
> namespace.cleanup_comments = function() {
> - // Cleanup previous inline review comments.
> - Y.all('.ict-header').each( function(line) {
> - line.next().remove(true);
> - line.remove(true);
> - });
> + // Cleanup existing inline review comments.
> + Y.all('.inline-comments').remove(true);
> };
>
> namespace.populate_comments = function() {
> @@ -350,7 +347,7 @@
> if (display) {
> css_display = 'table-row';
> }
> - Y.all('.ict-header').each( function() {
> + Y.all('.inline-comments').each( function() {
> this.setStyle('display', css_display);
> this.next().setStyle('display', css_display);
> });
>
> === modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
> --- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-05-15 03:47:40 +0000
> +++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js 2014-05-19 09:28:43 +0000
> @@ -81,28 +81,34 @@
> responseHeaders: {'Content-Type': 'application/json'}});
>
> // Published comment is displayed.
> - var header = Y.one('#diff-line-2').next();
> + var comments = Y.one('#diff-line-2').next().one('div');
> // XXX cprov 20140226: test disabled due to ES4 lack of
> // ISO8601 support. Although the vast majority of production
> // clients run ES5.
> //Y.Assert.areEqual(
> // 'Comment by Foo Bar (name16) on 2012-08-12',
> // header.get('text'));
> - var text = header.next();
> - Y.Assert.areEqual('This is preloaded.', text.get('text'));
> + var first = comments.one('div:first-child')
> + Y.Assert.areEqual(
> + 'This is preloaded.',
> + first.one('.boardCommentBody').get('text'));
>
> // Draft comment for line 2 is displayed after all published
> // comments.
> - header = text.next()
> - Y.Assert.areEqual('Draft comment', header.get('text'));
> - var text = header.next();
> - Y.Assert.areEqual('Boing!', text.get('text'));
> + var second = first.next()
> + Y.Assert.areEqual(
> + 'Draft comment',
> + second.one('.boardCommentDetails').get('text'));
> + Y.Assert.areEqual(
> + 'Boing!', second.one('.boardCommentBody').get('text'));
>
> // Draft comment for line 3 is displayed.
> - header = Y.one('#diff-line-3').next();
> - Y.Assert.areEqual('Draft comment', header.get('text'));
> - text = header.next();
> - Y.Assert.areEqual('Zoing!', text.get('text'));
> + var third = Y.one('#diff-line-3').next();
> + Y.Assert.areEqual(
> + 'Draft comment',
> + third.one('.boardCommentDetails').get('text'));
> + Y.Assert.areEqual(
> + 'Zoing!', third.one('.boardCommentBody').get('text'));
> },
>
> test_draft_handler: function() {
> @@ -115,25 +121,26 @@
> {io_provider: mockio});
>
> // No draft comment in line 1.
> - Y.Assert.isNull(Y.one('#ict-1-draft-header'));
> + Y.Assert.isNull(Y.one('#comments-diff-line-1 .draft'));
>
> // Let's create one.
> var line = Y.one('#diff-line-1');
> line.simulate('dblclick');
> - var ic_area = line.next().next();
> + var ic_area = Y.one('#comments-diff-line-1 .draft');
> ic_area.one('.yui3-ieditor-input>textarea').set('value', 'Go!');
> ic_area.one('.lazr-pos').simulate('click');
>
> // LP was hit and a comment was created.
> Y.Assert.areEqual(1, mockio.requests.length);
> Y.Assert.areEqual(
> - 'Draft comment', Y.one('#ict-1-draft-header').get('text'));
> + 'Draft comment',
> + ic_area.one('.boardCommentDetails').get('text'));
> Y.Assert.areEqual(
> 'Go!', ic_area.one('.yui3-editable_text-text').get('text'));
>
> // Cancelling a draft comment attempt ...
> line.simulate('dblclick');
> - ic_area = line.next().next();
> + ic_area = line.next();
> ic_area.one('.yui3-ieditor-input>textarea').set('value', 'No!');
> ic_area.one('.lazr-neg').simulate('click');
>
> @@ -144,14 +151,14 @@
>
> // Removing a draft comment by submitting an emtpy text.
> line.simulate('dblclick');
> - ic_area = line.next().next();
> + ic_area = line.next();
> ic_area.one('.yui3-ieditor-input>textarea').set('value', '');
> ic_area.one('.lazr-pos').simulate('click');
>
> // LP is hit again and the previous comment is removed,
> // the next row diplayed is the next diff line.
> Y.Assert.areEqual(2, mockio.requests.length);
> - Y.Assert.isNull(Y.one('#ict-1-draft-header'));
> + Y.Assert.isNull(Y.one('#comments-diff-line-1 .draft'));
> Y.Assert.areEqual('diff-line-2', line.next().get('id'));
> },
>
> @@ -161,7 +168,7 @@
> module.add_doubleclick_handler();
> var line = Y.one('#diff-line-1');
> line.simulate('dblclick');
> - ic_area = line.next().next();
> + ic_area = line.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'));
> @@ -395,17 +402,17 @@
> // Render a draft comment for tests.
> module.create_row({'line_number': '1', 'text': 'inline'});
> Y.Assert.areEqual(
> - 'table-row', Y.one('tr.ict-header').getStyle('display'));
> + 'table-row', Y.one('.inline-comments').getStyle('display'));
> ic_check.set('checked', false);
> ic_check.simulate('change');
> this.wait(function() {}, 1);
> Y.Assert.areEqual(
> - 'none', Y.one('tr.ict-header').getStyle('display'));
> + 'none', Y.one('.inline-comments').getStyle('display'));
> ic_check.set('checked', true);
> ic_check.simulate('change');
> this.wait(function() {}, 1);
> Y.Assert.areEqual(
> - 'table-row', Y.one('tr.ict-header').getStyle('display'));
> + 'table-row', Y.one('.inline-comments').getStyle('display'));
> },
>
> test_publish_drafts: function() {
>
> === modified file 'lib/lp/code/templates/branchmergeproposal-diff.pt'
> --- lib/lp/code/templates/branchmergeproposal-diff.pt 2012-11-28 21:48:48 +0000
> +++ lib/lp/code/templates/branchmergeproposal-diff.pt 2014-05-19 09:28:43 +0000
> @@ -15,7 +15,7 @@
> </li>
> </ul>
> </div>
> - <div class="boardCommentBody attachmentBody">
> + <div class="attachmentBody">
> <tal:diff replace="structure view/preview_diff_text/fmt:diff" />
> <tal:diff-not-available condition="not: view/diff_available">
> The diff is not available at this time. You can reload the
>
--
https://code.launchpad.net/~wgrant/launchpad/ic-js-cleanup/+merge/219531
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/ic-js-cleanup into lp:launchpad.
Follow ups
