opendev/gerrit
Code Issues Proposed changes

Merge "Display comment resolve state"

Browse Source
This commit is contained in:
Wyatt Allen
2017-01-10 00:38:40 +00:00
committed by Gerrit Code Review
parent b039ee53b0 8d1ac7e824
commit 866788ce05
6 changed files with 100 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -23,13 +23,18 @@ limitations under the License.
<template>
<style>
:host {
background-color: #ffd;
border: 1px solid #bbb;
display: block;
white-space: normal;
}
#container {
background-color: #fcfad6;
}
#container.unresolved {
background-color: #fcfaa6;
}
</style>
<div id="container">
<div id="container" class$="[[_computeHostClass(_unresolved)]]">
<template id="commentList" is="dom-repeat" items="[[_orderedComments]]" as="comment">
<gr-diff-comment
comment="{{comment}}"

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

@@ -45,6 +45,10 @@
_showActions: Boolean,
_orderedComments: Array,
_unresolved: {
type: Boolean,
notify: true,
},
},
behaviors: [
@@ -56,7 +60,7 @@
},
observers: [
'_commentsChanged(comments.splices)',
'_commentsChanged(comments.*)',
],
keyBindings: {
@@ -83,6 +87,7 @@
addDraft: function(opt_lineNum, opt_range) {
var draft = this._newDraft(opt_lineNum, opt_range);
draft.__editing = true;
draft.unresolved = true;
this.push('comments', draft);
},
@@ -92,6 +97,11 @@
_commentsChanged: function(changeRecord) {
this._orderedComments = this._sortedComments(this.comments);
this._unresolved = this._getLastComment().unresolved;
},
_getLastComment: function() {
return this._orderedComments[this._orderedComments.length - 1] || {};
},
_handleEKey: function(e) {
@@ -125,11 +135,13 @@
});
},
_createReplyComment: function(parent, content, opt_isEditing) {
_createReplyComment: function(parent, content, opt_isEditing,
opt_unresolved) {
var reply = this._newReply(
this._orderedComments[this._orderedComments.length - 1].id,
parent.line,
content);
content,
opt_unresolved);
// If there is currently a comment in an editing state, add an attribute
// so that the gr-diff-comment knows not to populate the draft text.
@@ -162,17 +174,17 @@
var msg = comment.message;
quoteStr = '> ' + msg.replace(NEWLINE_PATTERN, '\n> ') + '\n\n';
}
this._createReplyComment(comment, quoteStr, true);
this._createReplyComment(comment, quoteStr, true, comment.unresolved);
},
_handleCommentAck: function(e) {
var comment = e.detail.comment;
this._createReplyComment(comment, 'Ack');
this._createReplyComment(comment, 'Ack', false, comment.unresolved);
},
_handleCommentDone: function(e) {
var comment = e.detail.comment;
this._createReplyComment(comment, 'Done');
this._createReplyComment(comment, 'Done', false, false);
},
_handleCommentFix: function(e) {
@@ -180,7 +192,7 @@
var msg = comment.message;
var quoteStr = '> ' + msg.replace(NEWLINE_PATTERN, '\n> ') + '\n\n';
var response = quoteStr + 'Please Fix';
this._createReplyComment(comment, response);
this._createReplyComment(comment, response, false, true);
},
_commentElWithDraftID: function(id) {
@@ -193,12 +205,15 @@
return null;
},
_newReply: function(inReplyTo, opt_lineNum, opt_message) {
_newReply: function(inReplyTo, opt_lineNum, opt_message, opt_unresolved) {
var d = this._newDraft(opt_lineNum);
d.in_reply_to = inReplyTo;
if (opt_message != null) {
d.message = opt_message;
}
if (opt_unresolved !== undefined) {
d.unresolved = opt_unresolved;
}
return d;
},
@@ -261,7 +276,7 @@
console.error('Comment update for another comment thread.');
return;
}
this.comments[index] = comment;
this.set(['comments', index], comment);
},
_indexOf: function(comment, arr) {
@@ -274,5 +289,9 @@
}
return -1;
},
_computeHostClass: function(unresolved) {
return unresolved ? 'unresolved' : '';
},
});
})();

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

@@ -240,6 +240,7 @@ limitations under the License.
assert.equal(drafts.length, 1);
assert.equal(drafts[0].message, 'Done');
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
assert.isFalse(drafts[0].unresolved);
done();
});
commentEl.fire('create-done-comment', {comment: commentEl.comment},
@@ -259,6 +260,7 @@ limitations under the License.
assert.equal(
drafts[0].message, '> is this a crossover episode!?\n\nPlease Fix');
assert.equal(drafts[0].in_reply_to, 'baf0414d_60047215');
assert.isTrue(drafts[0].unresolved);
done();
});
commentEl.fire('create-fix-comment', {comment: commentEl.comment},
@@ -462,13 +464,15 @@ limitations under the License.
assert.strictEqual(element.comments[0], updatedComment);
});
test('orphan replies', function() {
var comments = [
suite('jack and sally comment data test consolidation', function() {
setup(function() {
element.comments = [
{
id: 'jacks_reply',
message: 'i like you, too',
in_reply_to: 'sallys_confession',
updated: '2015-12-25 15:00:20.396000000',
unresolved: false,
}, {
id: 'sallys_confession',
in_reply_to: 'nonexistent_comment',
@@ -484,70 +488,41 @@ limitations under the License.
message: 'i will poison you so i can get away',
updated: '2015-10-31 15:00:20.396000000',
}];
element.comments = comments;
assert.equal(4, element._orderedComments.length);
});
test('orphan replies', function() {
assert.equal(4, element._orderedComments.length);
});
test('keyboard shortcuts', function() {
var expandCollapseStub = sinon.stub(element, '_expandCollapseComments');
MockInteractions.pressAndReleaseKeyOn(element, 69, null, 'e');
assert.isTrue(expandCollapseStub.lastCall.calledWith(false));
MockInteractions.pressAndReleaseKeyOn(element, 69, 'shift', 'e');
assert.isTrue(expandCollapseStub.lastCall.calledWith(true));
expandCollapseStub.restore();
});
test('comment in_reply_to is either null or most recent comment id',
function() {
element._createReplyComment(element.comments[3], 'dummy', true);
flushAsynchronousOperations();
assert.equal(element._orderedComments.length, 5);
assert.equal(element._orderedComments[4].in_reply_to, 'jacks_reply');
});
test('resolvable comments', function() {
assert.isFalse(element._unresolved);
element._createReplyComment(element.comments[3], 'dummy', true, true);
flushAsynchronousOperations();
assert.isTrue(element._unresolved);
});
});
test('keyboard shortcuts', function() {
var comments = [
{
id: 'jacks_reply',
message: 'i like you, too',
in_reply_to: 'sallys_confession',
updated: '2015-12-25 15:00:20.396000000',
}, {
id: 'sallys_confession',
in_reply_to: 'nonexistent_comment',
message: 'i like you, jack',
updated: '2015-12-24 15:00:20.396000000',
}, {
id: 'sally_to_dr_finklestein',
in_reply_to: 'nonexistent_comment',
message: 'i’m running away',
updated: '2015-10-31 09:00:20.396000000',
}, {
id: 'sallys_defiance',
message: 'i will poison you so i can get away',
updated: '2015-10-31 15:00:20.396000000',
}];
element.comments = comments;
var expandCollapseStub = sinon.stub(element, '_expandCollapseComments');
MockInteractions.pressAndReleaseKeyOn(element, 69, null, 'e');
assert.isTrue(expandCollapseStub.lastCall.calledWith(false));
MockInteractions.pressAndReleaseKeyOn(element, 69, 'shift', 'e');
assert.isTrue(expandCollapseStub.lastCall.calledWith(true));
expandCollapseStub.restore();
});
test('comment in_reply_to is either null or most recent comment id',
function() {
var comments = [
{
id: 'jacks_reply',
message: 'i like you, too',
in_reply_to: 'sallys_confession',
updated: '2015-12-25 15:00:20.396000000',
}, {
id: 'sallys_confession',
in_reply_to: 'nonexistent_comment',
message: 'i like you, jack',
updated: '2015-12-24 15:00:20.396000000',
}, {
id: 'sally_to_dr_finklestein',
in_reply_to: 'nonexistent_comment',
message: 'i’m running away',
updated: '2015-10-31 09:00:20.396000000',
}, {
id: 'sallys_defiance',
message: 'i will poison you so i can get away',
updated: '2015-10-31 15:00:20.396000000',
}];
element.set('comments', comments);
element._createReplyComment(comments[3], 'dummy', true);
flushAsynchronousOperations();
assert.equal(element._orderedComments.length, 5);
assert.equal(element._orderedComments[4].in_reply_to, 'jacks_reply');
test('_computeHostClass', function() {
assert.equal(element._computeHostClass(true), 'unresolved');
assert.equal(element._computeHostClass(false), '');
});
});
</script>

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

@@ -109,7 +109,8 @@ limitations under the License.
display: inline;
}
.draft:not(.editing) .save,
.draft:not(.editing) .cancel {
.draft:not(.editing) .cancel,
.draft:not(.editing) .resolve {
display: none;
}
.editing .message,
@@ -170,6 +171,13 @@ limitations under the License.
#container.collapsed iron-autogrow-textarea {
display: none;
}
.resolve {
margin: auto;
}
.resolve label {
color: #333;
font-size: 12px;
}
</style>
<div id="container"
class="container"

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

@@ -114,6 +114,11 @@
value: '',
observer: '_messageTextChanged',
},
resolved: {
type: Boolean,
observer: '_toggleResolved',
},
},
observers: [
@@ -186,6 +191,7 @@
_commentChanged: function(comment) {
this.editing = !!comment.__editing;
this.resolved = !comment.unresolved;
if (this.editing) { // It's a new draft/reply, notify.
this._fireUpdate();
}
@@ -354,6 +360,7 @@
_handleSave: function(e) {
e.preventDefault();
this.set('comment.__editing', false);
this.save();
},
@@ -439,5 +446,14 @@
_handleMouseLeave: function(e) {
this.fire('comment-mouse-out', this._getEventPayload());
},
_handleToggleResolved: function() {
this.resolved = !this.resolved;
},
_toggleResolved: function(resolved) {
this.comment.unresolved = !resolved;
this.fire('comment-update', this._getEventPayload());
},
});
})();

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

@@ -129,13 +129,6 @@ limitations under the License.
MockInteractions.tap(element.$$('.done'));
});
test('proper event fires on fix', function(done) {
element.addEventListener('create-fix-comment', function(e) {
done();
});
MockInteractions.tap(element.$$('.fix'));
});
test('clicking on date link does not trigger nav', function() {
var showStub = sinon.stub(page, 'show');
var dateEl = element.$$('.date');
@@ -480,6 +473,7 @@ limitations under the License.
line: 5,
path: '/path/to/file',
message: 'good news, everyone!',
unresolved: false,
},
patchNum: 1,
},