opendev/gerrit
Code Issues Proposed changes

Move reply buttons to comment thread

Browse Source 
Move all buttons that generate a reply of some sort (done, ack, reply,
quote) to the comment thread instead of the comment [1].

When there is a draft for a particular comment thread, all reply buttons
are hidden [2].  For example, if you click reply, you cannot click done
on the same thread, unless you remove the draft.

Each thread can have up to 1 draft. It's also worth noting that if a
thread has a draft, and the user clicks on the line or 'c' at the same
range, the existing draft will switch to 'editing' form.

[1] With the exception of "please fix" for robot comments.
[2] In this case, The please fix button will be disabled when other
reply buttons are hidden.

Feature: Issue 5410
Change-Id: Id847ee0cba0d0ce4e5b6476f58141866d41ffdad
This commit is contained in:
Becky Siegel
2017-02-09 16:07:32 -08:00
parent ddc6b7160f
commit 2c602323e1
11 changed files with 172 additions and 217 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
View File

@@ -24,6 +24,9 @@ limitations under the License.
display: block;
white-space: normal;
}
gr-diff-comment-thread + gr-diff-comment-thread {
margin-top: .2em;
}
</style>
<template is="dom-repeat" items="[[_threadGroups]]"
as="thread">

8
polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
View File

@@ -74,6 +74,14 @@
_sortByDate: function(threadGroups) {
if (!threadGroups.length) { return; }
return threadGroups.sort(function(a, b) {
// If a comment is a draft, it doesn't have a start_datetime yet.
// Assume it is newer than the comment it is being compared to.
if (!a.start_datetime) {
return 1;
}
if (!b.start_datetime) {
return -1;
}
return util.parseDate(a.start_datetime) -
util.parseDate(b.start_datetime);
});

26
polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
View File

@@ -165,6 +165,32 @@ limitations under the License.
];
assert.deepEqual(element._sortByDate(threadGroups), expectedResult);
// When a comment doesn't have a date, the one without the date should be
// last.
var threadGroups = [
{
start_datetime: '2015-12-23 15:00:20.396000000',
comments: [],
locationRange: 'line',
},
{
comments: [],
locationRange: 'range-1-1-1-2',
},
];
var expectedResult = [
{
start_datetime: '2015-12-23 15:00:20.396000000',
comments: [],
locationRange: 'line',
},
{
comments: [],
locationRange: 'range-1-1-1-2',
},
];
});
test('_calculateLocationRange', function() {

24
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.html
View File

@@ -28,6 +28,10 @@ limitations under the License.
margin-bottom: 1px;
white-space: normal;
}
.actions {
border-top: 1px dotted #bbb;
padding: .5em .7em;
}
#container {
background-color: #fcfad6;
}
@@ -36,21 +40,31 @@ limitations under the License.
}
</style>
<div id="container" class$="[[_computeHostClass(_unresolved)]]">
<template id="commentList" is="dom-repeat" items="[[_orderedComments]]" as="comment">
<template id="commentList" is="dom-repeat" items="[[_orderedComments]]"
as="comment">
<gr-diff-comment
comment="{{comment}}"
robot-button-disabled="[[_hideActions(_showActions, _lastComment)]]"
change-num="[[changeNum]]"
patch-num="[[patchNum]]"
draft="[[comment.__draft]]"
show-actions="[[_showActions]]"
comment-side="[[comment.__commentSide]]"
project-config="[[projectConfig]]"
on-comment-discard="_handleCommentDiscard"
on-create-ack-comment="_handleCommentAck"
on-create-done-comment="_handleCommentDone"
on-create-fix-comment="_handleCommentFix"
on-create-reply-comment="_handleCommentReply"></gr-diff-comment>
on-comment-discard="_handleCommentDiscard"></gr-diff-comment>
</template>
<div class="actions"
hidden$="[[_hideActions(_showActions, _lastComment)]]">
<gr-button id="replyBtn" class="action reply"
on-tap="_handleCommentReply">Reply</gr-button>
<gr-button id="quoteBtn" class="action quote"
on-tap="_handleCommentQuote">Quote</gr-button>
<gr-button id="ackBtn" class="action ack" on-tap="_handleCommentAck">
Ack</gr-button>
<gr-button id="doneBtn" class="action done" on-tap="_handleCommentDone">
Done</gr-button>
</div>
</div>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-storage id="storage"></gr-storage>

45
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
View File

@@ -47,6 +47,7 @@
},
_showActions: Boolean,
_lastComment: Object,
_orderedComments: Array,
_unresolved: {
type: Boolean,
@@ -77,16 +78,21 @@
this._setInitialExpandedState();
},
addOrEditDraft: function(opt_lineNum) {
var lastComment = this.comments[this.comments.length - 1];
if (lastComment && lastComment.__draft) {
addOrEditDraft: function(opt_lineNum, opt_range) {
var lastComment = this.comments[this.comments.length - 1] || {};
if (lastComment.__draft) {
var commentEl = this._commentElWithDraftID(
lastComment.id || lastComment.__draftID);
commentEl.editing = true;
// If the comment was collapsed, re-open it to make it clear which
// actions are available.
commentEl.collapsed = false;
} else {
this.addDraft(opt_lineNum,
lastComment ? lastComment.range : undefined,
lastComment ? lastComment.unresolved : undefined);
var range = opt_range ? opt_range :
lastComment ? lastComment.range : undefined;
var unresolved = lastComment ? lastComment.unresolved : undefined;
this.addDraft(opt_lineNum, range, unresolved);
}
},
@@ -103,7 +109,14 @@
_commentsChanged: function(changeRecord) {
this._orderedComments = this._sortedComments(this.comments);
this._unresolved = this._getLastComment().unresolved;
if (this._orderedComments.length) {
this._lastComment = this._getLastComment();
this._unresolved = this._lastComment.unresolved;
}
},
_hideActions: function(_showActions, _lastComment) {
return !_showActions || !_lastComment || !!_lastComment.__draft;
},
_getLastComment: function() {
@@ -192,23 +205,31 @@
}
},
_handleCommentReply: function(e) {
var comment = e.detail.comment;
_processCommentReply: function(opt_quote) {
var comment = this._lastComment;
var quoteStr;
if (e.detail.quote) {
if (opt_quote) {
var msg = comment.message;
quoteStr = '> ' + msg.replace(NEWLINE_PATTERN, '\n> ') + '\n\n';
}
this._createReplyComment(comment, quoteStr, true, comment.unresolved);
},
_handleCommentReply: function(e) {
this._processCommentReply();
},
_handleCommentQuote: function(e) {
this._processCommentReply(true);
},
_handleCommentAck: function(e) {
var comment = e.detail.comment;
var comment = this._lastComment;
this._createReplyComment(comment, 'Ack', false, comment.unresolved);
},
_handleCommentDone: function(e) {
var comment = e.detail.comment;
var comment = this._lastComment;
this._createReplyComment(comment, 'Done', false, false);
},

162
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
View File

@@ -43,6 +43,7 @@ limitations under the License.
var sandbox;
setup(function() {
sandbox = sinon.sandbox.create();
stub('gr-rest-api-interface', {
getLoggedIn: function() { return Promise.resolve(false); },
});
@@ -147,6 +148,17 @@ limitations under the License.
assert.isFalse(commentElStub.called);
assert.isTrue(addDraftStub.called);
});
test('_hideActions', function() {
var showActions = true;
var lastComment = {};
assert.equal(element._hideActions(showActions, lastComment), false);
showActions = false;
assert.equal(element._hideActions(showActions, lastComment), true);
var showActions = true;
lastComment.__draft = true;
assert.equal(element._hideActions(showActions, lastComment), true);
});
});
suite('comment action tests', function() {
@@ -189,7 +201,11 @@ limitations under the License.
test('reply', function(done) {
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('create-reply-comment', function() {
var replyBtn = element.$.replyBtn;
MockInteractions.tap(replyBtn);
flushAsynchronousOperations();
var drafts = element._orderedComments.filter(function(c) {
return c.__draft == true;
});
@@ -198,14 +214,15 @@ limitations under the License.
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
done();
});
commentEl.fire('create-reply-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('quote reply', function(done) {
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('create-reply-comment', function() {
var quoteBtn = element.$.quoteBtn;
MockInteractions.tap(quoteBtn);
flushAsynchronousOperations();
var drafts = element._orderedComments.filter(function(c) {
return c.__draft == true;
});
@@ -214,9 +231,6 @@ limitations under the License.
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
done();
});
commentEl.fire('create-reply-comment', {comment: commentEl.comment,
quote: true}, {bubbles: false});
});
test('quote reply multiline', function(done) {
element.comments = [{
@@ -233,7 +247,11 @@ limitations under the License.
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('create-reply-comment', function() {
var quoteBtn = element.$.quoteBtn;
MockInteractions.tap(quoteBtn);
flushAsynchronousOperations();
var drafts = element._orderedComments.filter(function(c) {
return c.__draft == true;
});
@@ -243,17 +261,18 @@ limitations under the License.
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
done();
});
commentEl.fire('create-reply-comment', {comment: commentEl.comment,
quote: true}, {bubbles: false});
});
test('ack', function(done) {
element.changeNum = '42';
element.patchNum = '1';
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('create-ack-comment', function() {
var drafts = element._orderedComments.filter(function(c) {
var ackBtn = element.$.ackBtn;
MockInteractions.tap(ackBtn);
flush(function() {
var drafts = element.comments.filter(function(c) {
return c.__draft == true;
});
assert.equal(drafts.length, 1);
@@ -261,8 +280,6 @@ limitations under the License.
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
done();
});
commentEl.fire('create-ack-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('done', function(done) {
@@ -270,8 +287,11 @@ limitations under the License.
element.patchNum = '1';
var commentEl = element.$$('gr-diff-comment');
assert.ok(commentEl);
commentEl.addEventListener('create-done-comment', function() {
var drafts = element._orderedComments.filter(function(c) {
var doneBtn = element.$.doneBtn;
MockInteractions.tap(doneBtn);
flush(function() {
var drafts = element.comments.filter(function(c) {
return c.__draft == true;
});
assert.equal(drafts.length, 1);
@@ -280,8 +300,6 @@ limitations under the License.
assert.isFalse(drafts[0].unresolved);
done();
});
commentEl.fire('create-done-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('please fix', function(done) {
@@ -328,7 +346,7 @@ limitations under the License.
});
test('first editing comment does not add __otherEditing attribute',
function(done) {
function() {
var commentEl = element.$$('gr-diff-comment');
element.comments = [{
author: {
@@ -341,101 +359,16 @@ limitations under the License.
updated: '2015-12-08 19:48:33.843000000',
__draft: true,
}];
var replyBtn = element.$.replyBtn;
MockInteractions.tap(replyBtn);
flushAsynchronousOperations();
commentEl.addEventListener('create-reply-comment', function() {
var editing = element._orderedComments.filter(function(c) {
return c.__editing == true;
});
assert.equal(editing.length, 1);
assert.equal(!!editing[0].__otherEditing, false);
done();
});
commentEl.fire('create-reply-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('two editing comments adds __otherEditing attribute', function(done) {
var commentEl = element.$$('gr-diff-comment');
element.comments = [{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
id: 'baf0414d_60047215',
line: 5,
message: 'is this a crossover episode!?',
updated: '2015-12-08 19:48:33.843000000',
__editing: true,
__draft: true,
}];
flushAsynchronousOperations();
commentEl.addEventListener('create-reply-comment', function() {
var editing = element._orderedComments.filter(function(c) {
return c.__editing == true;
});
assert.equal(editing.length, 2);
assert.equal(editing[1].__otherEditing, true);
done();
});
commentEl.fire('create-reply-comment', {comment: commentEl.comment},
{bubbles: false});
});
test('When editing other comments, local storage set after discard',
function(done) {
element.changeNum = '42';
element.patchNum = '1';
element.comments = [{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
id: 'baf0414d_60047215',
in_reply_to: 'baf0414d_60047215',
line: 5,
message: 'is this a crossover episode!?',
updated: '2015-12-08 19:48:31.843000000',
},
{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
__draftID: '1',
in_reply_to: 'baf0414d_60047215',
line: 5,
message: 'yes',
updated: '2015-12-08 19:48:32.843000000',
__draft: true,
__editing: true,
},
{
author: {
name: 'Mr. Peanutbutter',
email: 'tenn1sballchaser@aol.com',
},
__draftID: '2',
in_reply_to: 'baf0414d_60047215',
line: 5,
message: 'no',
updated: '2015-12-08 19:48:33.843000000',
__draft: true,
__editing: true,
}];
var storageStub = sinon.stub(element.$.storage, 'setDraftComment');
flushAsynchronousOperations();
var draftEl =
Polymer.dom(element.root).querySelectorAll('gr-diff-comment')[1];
assert.ok(draftEl);
draftEl.addEventListener('comment-discard', function() {
assert.isTrue(storageStub.called);
storageStub.restore();
done();
});
draftEl.fire('comment-discard', null, {bubbles: false});
});
test('When not editing other comments, local storage not set after discard',
@@ -602,5 +535,16 @@ limitations under the License.
var draft = element._newDraft();
assert.equal(draft.__commentSide, 'left');
});
test('new comment gets created', function() {
element.comments = [];
element.addOrEditDraft(1);
assert.equal(element.comments.length, 1);
// Mock a submitted comment.
element.comments[0].__draft = false;
element.comments[0].id = element.comments[0].__draftID;
element.addOrEditDraft(1);
assert.equal(element.comments.length, 2);
});
});
</script>

12
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
View File

@@ -134,7 +134,7 @@ limitations under the License.
margin-top: -.4em;
}
.runIdInformation {
margin-bottom: .5em;
margin: 1em 0;
}
.robotRun {
margin-left: .5em;
@@ -218,6 +218,7 @@ limitations under the License.
on-keydown="_handleTextareaKeydown"></iron-autogrow-textarea>
<gr-formatted-text class="message"
content="[[comment.message]]"
no-trailing-margin="[[!comment.__draft]]"
collapsed="[[collapsed]]"
config="[[projectConfig.commentlinks]]"></gr-formatted-text>
<div hidden$="[[!comment.robot_run_id]]">
@@ -229,11 +230,6 @@ limitations under the License.
</div>
</div>
<div class="actions humanActions" hidden$="[[!_showHumanActions]]">
<gr-button class="action reply" on-tap="_handleReply">Reply</gr-button>
<gr-button class="action quote" on-tap="_handleQuote">Quote</gr-button>
<gr-button class="action ack" on-tap="_handleAck">Ack</gr-button>
<gr-button class="action done" on-tap="_handleDone">
Done</gr-button>
<gr-button class="action edit hideOnPublished" on-tap="_handleEdit">
Edit</gr-button>
<gr-button class="action save hideOnPublished" on-tap="_handleSave"
@@ -254,7 +250,9 @@ limitations under the License.
</div>
</div>
<div class="actions robotActions" hidden$="[[!_showRobotActions]]">
<gr-button class="action fix" on-tap="_handleFix">
<gr-button class="action fix"
on-tap="_handleFix"
disabled="[[robotButtonDisabled]]">
Please Fix
</gr-button>
</div>

19
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
View File

@@ -19,24 +19,6 @@
Polymer({
is: 'gr-diff-comment',
/**
* Fired when the Reply action is triggered.
*
* @event create-reply-comment
*/
/**
* Fired when the Ack action is triggered.
*
* @event create-ack-comment
*/
/**
* Fired when the Done action is triggered.
*
* @event create-done-comment
*/
/**
* Fired when the create fix comment action is triggered.
*
@@ -107,6 +89,7 @@
observer: '_toggleCollapseClass',
},
projectConfig: Object,
robotButtonDisabled: Boolean,
_xhrPromise: Object, // Used for testing.
_messageText: {

43
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
View File

@@ -98,37 +98,6 @@ limitations under the License.
'header middle content is not visible');
});
test('proper event fires on reply', function(done) {
element.addEventListener('create-reply-comment', function(e) {
assert.ok(e.detail.comment);
done();
});
MockInteractions.tap(element.$$('.reply'));
});
test('proper event fires on quote', function(done) {
element.addEventListener('create-reply-comment', function(e) {
assert.ok(e.detail.comment);
assert.isTrue(e.detail.quote);
done();
});
MockInteractions.tap(element.$$('.quote'));
});
test('proper event fires on ack', function(done) {
element.addEventListener('create-ack-comment', function(e) {
done();
});
MockInteractions.tap(element.$$('.ack'));
});
test('proper event fires on done', function(done) {
element.addEventListener('create-done-comment', function(e) {
done();
});
MockInteractions.tap(element.$$('.done'));
});
test('clicking on date link does not trigger nav', function() {
var showStub = sinon.stub(page, 'show');
var dateEl = element.$$('.date');
@@ -286,10 +255,6 @@ limitations under the License.
assert.isTrue(isVisible(element.$$('.discard')), 'discard is visible');
assert.isFalse(isVisible(element.$$('.save')), 'save is not visible');
assert.isFalse(isVisible(element.$$('.cancel')), 'cancel is not visible');
assert.isFalse(isVisible(element.$$('.reply')), 'reply is not visible');
assert.isFalse(isVisible(element.$$('.quote')), 'quote is not visible');
assert.isFalse(isVisible(element.$$('.ack')), 'ack is not visible');
assert.isFalse(isVisible(element.$$('.done')), 'done is not visible');
assert.isFalse(isVisible(element.$$('.resolve')),
'resolve is not visible');
assert.isFalse(element.$$('.humanActions').hasAttribute('hidden'));
@@ -300,10 +265,6 @@ limitations under the License.
assert.isTrue(isVisible(element.$$('.discard')), 'discard is visible');
assert.isTrue(isVisible(element.$$('.save')), 'save is visible');
assert.isFalse(isVisible(element.$$('.cancel')), 'cancel is visible');
assert.isFalse(isVisible(element.$$('.reply')), 'reply is not visible');
assert.isFalse(isVisible(element.$$('.quote')), 'quote is not visible');
assert.isFalse(isVisible(element.$$('.ack')), 'ack is not visible');
assert.isFalse(isVisible(element.$$('.done')), 'done is not visible');
assert.isTrue(isVisible(element.$$('.resolve')), 'resolve is visible');
assert.isFalse(element.$$('.humanActions').hasAttribute('hidden'));
assert.isTrue(element.$$('.robotActions').hasAttribute('hidden'));
@@ -315,10 +276,6 @@ limitations under the License.
'discard is not visible');
assert.isFalse(isVisible(element.$$('.save')), 'save is not visible');
assert.isFalse(isVisible(element.$$('.cancel')), 'cancel is not visible');
assert.isTrue(isVisible(element.$$('.reply')), 'reply is visible');
assert.isTrue(isVisible(element.$$('.quote')), 'quote is visible');
assert.isTrue(isVisible(element.$$('.ack')), 'ack is visible');
assert.isTrue(isVisible(element.$$('.done')), 'done is visible');
assert.isFalse(element.$$('.humanActions').hasAttribute('hidden'));
assert.isTrue(element.$$('.robotActions').hasAttribute('hidden'));

2
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
View File

@@ -230,7 +230,7 @@
var threadEl = this._getOrCreateThreadAtLineRange(contentEl, patchNum,
diffSide, side, range);
threadEl.addDraft(line, range);
threadEl.addOrEditDraft(line, range);
},
_addDraft: function(lineEl, opt_lineNum) {

1
polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.html
View File

@@ -45,6 +45,7 @@ limitations under the License.
gr-linked-text.pre {
font-family: var(--monospace-font-family);
}
</style>
<div id="container"></div>
</template>