Draft comments initial implementation
This implements basic functionality for draft CRUD operations.
There are a few things that are TBD:
+ Layout edge cases within the diff view.
+ Reply/Done actions in threads.
+ Not allowing the user to add drafts if logged out.
+ I’m sure a few more things...
Feature: Issue 3649
Change-Id: Ia7419eecee5d5b20e73e17241990d7a7ffede0e8
diff --git a/lib/js/BUCK b/lib/js/BUCK
index d03753e..060788c 100644
--- a/lib/js/BUCK
+++ b/lib/js/BUCK
@@ -105,6 +105,21 @@
)
bower_component(
+ name = 'iron-autogrow-textarea',
+ package = 'polymerelements/iron-autogrow-textarea',
+ version = '1.0.10',
+ deps = [
+ ':iron-behaviors',
+ ':iron-flex-layout',
+ ':iron-form-element-behavior',
+ ':iron-validatable-behavior',
+ ':polymer',
+ ],
+ license = 'polymer',
+ sha1 = 'd368240e60a4b02ffc731ad8f45f3c8bbf47e9bd',
+)
+
+bower_component(
name = 'iron-behaviors',
package = 'polymerelements/iron-behaviors',
version = '1.0.11',
@@ -151,6 +166,15 @@
)
bower_component(
+ name = 'iron-form-element-behavior',
+ package = 'polymerelements/iron-form-element-behavior',
+ version = '1.0.6',
+ deps = [':polymer'],
+ license = 'polymer',
+ sha1 = '8d9e6530edc1b99bec1a5c34853911fba3701220',
+)
+
+bower_component(
name = 'iron-input',
package = 'polymerelements/iron-input',
version = '1.0.6',
@@ -206,7 +230,6 @@
name = 'iron-test-helpers',
package = 'polymerelements/iron-test-helpers',
version = '1.0.6',
- semver = '~1.0.6',
deps = [':polymer'],
license = 'DO_NOT_DISTRIBUTE',
sha1 = 'c0f7c7f010ca3c63fb08ae0d9462e400380cde2c',
diff --git a/polygerrit-ui/.gitignore b/polygerrit-ui/.gitignore
index af8e683..73b5221 100644
--- a/polygerrit-ui/.gitignore
+++ b/polygerrit-ui/.gitignore
@@ -1,5 +1,6 @@
node_modules
npm-debug.log
dist
+bower.json
bower_components
.tmp
diff --git a/polygerrit-ui/BUCK b/polygerrit-ui/BUCK
index af42cab..15b11f5 100644
--- a/polygerrit-ui/BUCK
+++ b/polygerrit-ui/BUCK
@@ -5,6 +5,7 @@
deps = [
'//lib/js:iron-a11y-keys-behavior',
'//lib/js:iron-ajax',
+ '//lib/js:iron-autogrow-textarea',
'//lib/js:iron-dropdown',
'//lib/js:iron-input',
'//lib/js:page',
diff --git a/polygerrit-ui/app/elements/gr-diff-comment-thread.html b/polygerrit-ui/app/elements/gr-diff-comment-thread.html
index b908cf7..5454914 100644
--- a/polygerrit-ui/app/elements/gr-diff-comment-thread.html
+++ b/polygerrit-ui/app/elements/gr-diff-comment-thread.html
@@ -27,7 +27,12 @@
}
</style>
<template id="commentList" is="dom-repeat" items="{{_orderedComments}}" as="comment">
- <gr-diff-comment comment="[[comment]]"></gr-diff-comment>
+ <gr-diff-comment
+ comment="{{comment}}"
+ change-num="[[changeNum]]"
+ patch-num="[[patchNum]]"
+ draft="[[comment.__draft]]"
+ editing="[[!comment.message]]"></gr-diff-comment>
</template>
</template>
<script>
@@ -44,17 +49,31 @@
* @event gr-diff-comment-thread-height-changed
*/
+ /**
+ * Fired when the thread should be discarded.
+ *
+ * @event gr-diff-comment-thread-discard
+ */
+
properties: {
+ changeNum: String,
comments: {
type: Array,
+ value: function() { return []; },
observer: '_commentsChanged',
},
+ patchNum: String,
+
_orderedComments: Array,
},
ready: function() {
this.addEventListener('gr-diff-comment-height-changed',
this._handleCommentHeightChange.bind(this));
+ this.addEventListener('gr-diff-comment-reply',
+ this._handleCommentReply.bind(this));
+ this.addEventListener('gr-diff-comment-discard',
+ this._handleCommentDiscard.bind(this));
},
_commentsChanged: function(comments) {
@@ -62,7 +81,6 @@
},
_sortedComments: function(comments) {
- var comments = comments || [];
comments.sort(function(c1, c2) {
return util.parseDate(c1.updated) - util.parseDate(c2.updated);
});
@@ -105,6 +123,37 @@
{height: this.offsetHeight});
},
+ _handleCommentReply: function(e) {
+ console.log('should add reply...')
+ },
+
+ _handleCommentDiscard: function(e) {
+ var diffCommentEl = e.target;
+ var idx = this._indexOf(diffCommentEl.comment, this.comments);
+ if (idx == -1) {
+ throw Error('Cannot find comment ' +
+ JSON.stringify(diffCommentEl.comment));
+ }
+ this.comments.splice(idx, 1);
+ this._commentsChanged(this.comments);
+ if (this.comments.length == 0 && this.parentNode) {
+ this.parentNode.removeChild(this);
+ }
+ this.fire('gr-diff-comment-thread-height-changed',
+ {height: this.offsetHeight});
+ },
+
+ _indexOf: function(comment, arr) {
+ for (var i = 0; i < arr.length; i++) {
+ var c = arr[i];
+ if ((c.__draftID != null && c.__draftID == comment.__draftID) ||
+ (c.id != null && c.id == comment.id)) {
+ return i;
+ }
+ }
+ return -1;
+ },
+
});
})();
</script>
diff --git a/polygerrit-ui/app/elements/gr-diff-comment.html b/polygerrit-ui/app/elements/gr-diff-comment.html
index 343f7d3..f137996 100644
--- a/polygerrit-ui/app/elements/gr-diff-comment.html
+++ b/polygerrit-ui/app/elements/gr-diff-comment.html
@@ -15,6 +15,9 @@
-->
<link rel="import" href="../bower_components/polymer/polymer.html">
+<link rel="import" href="../bower_components/polymer/polymer.html">
+<link rel="import" href="../bower_components/iron-autogrow-textarea/iron-autogrow-textarea.html">
+<link rel="import" href="../bower_components/iron-ajax/iron-request.html">
<link rel="import" href="gr-date-formatter.html">
<dom-module id="gr-diff-comment">
@@ -24,6 +27,12 @@
border: 1px solid #ddd;
display: block;
}
+ :host([disabled]) {
+ pointer-events: none;
+ }
+ :host([disabled]) .container {
+ opacity: .5;
+ }
.header,
.message,
.actions {
@@ -31,32 +40,113 @@
}
.header {
background-color: #eee;
+ display: flex;
font-family: 'Open Sans', sans-serif;
}
- gr-date-formatter {
- float: right;
+ .headerLeft {
+ flex: 1;
+ }
+ .authorName,
+ .draftLabel {
+ font-weight: bold;
+ }
+ .draftLabel {
+ color: #999;
+ display: none;
+ }
+ .date {
+ justify-content: flex-end;
margin-left: 5px;
}
- .authorName {
- font-weight: bold;
+ a.date:link,
+ a.date:visited {
+ color: #666;
+ text-decoration: none;
+ }
+ a.date:hover {
+ text-decoration: underline;
}
.message {
white-space: pre-wrap;
}
.actions {
- /** TODO: remove once the actions actually do something. **/
- display: none;
+ display: flex;
padding-top: 0;
}
+ .action {
+ margin-right: 1em;
+ }
+ .action[disabled] {
+ opacity: .5;
+ pointer-events: none;
+ }
+ .danger {
+ display: flex;
+ flex: 1;
+ justify-content: flex-end;
+ }
+ .editMessage {
+ display: none;
+ margin: .5em .7em;
+ width: calc(100% - 1.4em - 2px);
+ }
+ .danger .action {
+ margin-right: 0;
+ }
+ .container:not(.draft) .actions :not(.reply):not(.done) {
+ display: none;
+ }
+ .draft .reply,
+ .draft .done {
+ display: none;
+ }
+ .draft .draftLabel {
+ display: inline;
+ }
+ .draft:not(.editing) .save,
+ .draft:not(.editing) .cancel {
+ display: none;
+ }
+ .editing .message,
+ .editing .reply,
+ .editing .done,
+ .editing .edit {
+ display: none;
+ }
+ .editing .editMessage {
+ display: block;
+ }
</style>
- <div class="header" id="header">
- <span class="authorName">[[comment.author.name]]</span>
- <gr-date-formatter date-str="[[comment.updated]]"></gr-date-formatter>
- </div>
- <div class="message">[[comment.message]]</div>
- <div class="actions">
- <a class="reply" href="#" on-tap="_handleReply">Reply</a>
- <a class="done" href="#" on-tap="_handleDone">Done</a>
+ <div class="container" id="container">
+ <div class="header" id="header">
+ <div class="headerLeft">
+ <span class="authorName">[[comment.author.name]]</span>
+ <span class="draftLabel">DRAFT</span>
+ </div>
+ <a class="date" href$="[[_computeLinkToComment(comment)]]" on-tap="_handleLinkTap">
+ <gr-date-formatter date-str="[[comment.updated]]"></gr-date-formatter>
+ </a>
+ </div>
+ <iron-autogrow-textarea
+ id="editTextarea"
+ class="editMessage"
+ disabled="{{disabled}}"
+ rows="4"
+ max-rows="4"
+ bind-value="{{_editDraft}}"></iron-autogrow-textarea>
+ <div class="message">[[comment.message]]</div>
+ <div class="actions">
+ <a class="action reply" href="#" on-tap="_handleReply">Reply</a>
+ <a class="action done" href="#" on-tap="_handleDone">Done</a>
+ <a class="action edit" href="#" on-tap="_handleEdit">Edit</a>
+ <a class="action save" href="#"
+ disabled$="[[_computeSaveDisabled(_editDraft)]]"
+ on-tap="_handleSave">Save</a>
+ <a class="action cancel" href="#" on-tap="_handleCancel">Cancel</a>
+ <div class="danger">
+ <a class="action discard" href="#" on-tap="_handleDiscard">Discard</a>
+ </div>
+ </div>
</div>
</template>
<script>
@@ -84,17 +174,81 @@
* @event gr-diff-comment-done
*/
+ /**
+ * Fired when this comment is discarded.
+ *
+ * @event gr-diff-comment-discard
+ */
+
properties: {
- comment: Object,
+ changeNum: String,
+ comment: {
+ type: Object,
+ notify: true,
+ },
+ disabled: {
+ type: Boolean,
+ value: false,
+ reflectToAttribute: true,
+ },
draft: {
type: Boolean,
value: false,
+ observer: '_draftChanged',
},
+ editing: {
+ type: Boolean,
+ value: false,
+ observer: '_editingChanged',
+ },
+ patchNum: String,
+
+ _xhrPromise: Object,
+ _editDraft: String,
},
attached: function() {
- this.fire('gr-diff-comment-height-changed',
+ this._heightChanged();
+ },
+
+ _heightChanged: function() {
+ this.async(function() {
+ this.fire('gr-diff-comment-height-changed',
{height: this.offsetHeight});
+ }.bind(this));
+ },
+
+ _draftChanged: function(draft) {
+ this.$.container.classList.toggle('draft', draft);
+ },
+
+ _editingChanged: function(editing) {
+ this.$.container.classList.toggle('editing', editing);
+ if (editing) {
+ this.async(function() {
+ this.$.editTextarea.textarea.focus();
+ }.bind(this));
+ }
+ this._heightChanged();
+ },
+
+ _computeLinkToComment: function(comment) {
+ return '#' + comment.line;
+ },
+
+ _computeSaveDisabled: function(draft) {
+ return draft == null || draft.trim() == '';
+ },
+
+ _handleLinkTap: function(e) {
+ e.preventDefault();
+ var hash = this._computeLinkToComment(this.comment);
+ // Don't add the hash to the window history if it's already there.
+ // Otherwise you mess up expected back button behavior.
+ if (window.location.hash == hash) { return; }
+ // Change the URL but don’t trigger a nav event. Otherwise it will
+ // reload the page.
+ page.show(window.location.pathname + hash, null, false);
},
_handleReply: function(e) {
@@ -107,6 +261,89 @@
this.fire('gr-diff-comment-done');
},
+ _handleEdit: function(e) {
+ e.preventDefault();
+ this._editDraft = this.comment.message;
+ this.editing = true;
+ },
+
+ _handleSave: function(e) {
+ e.preventDefault();
+ this.comment.message = this._editDraft;
+ this.disabled = true;
+ var endpoint = this._restEndpoint(this.comment.id);
+ this._send('PUT', endpoint).then(function(req) {
+ this.disabled = false;
+ var comment = req.response;
+ comment.__draft = true;
+ // Maintain the ephemeral draft ID for identification by other
+ // elements.
+ if (this.comment.__draftID) {
+ comment.__draftID = this.comment.__draftID;
+ }
+ this.comment = comment;
+ this.editing = false;
+ }.bind(this)).catch(function(err) {
+ alert('Your draft couldn’t be saved. Check the console and contact ' +
+ 'the PolyGerrit team for assistance.');
+ this.disabled = false;
+ }.bind(this));
+ },
+
+ _handleCancel: function(e) {
+ e.preventDefault();
+ if (this.comment.message == null || this.comment.message.length == 0) {
+ this.fire('gr-diff-comment-discard');
+ return;
+ }
+ this._editDraft = this.comment.message;
+ this.editing = false;
+ },
+
+ _handleDiscard: function(e) {
+ e.preventDefault();
+ if (!this.comment.__draft) {
+ throw Error('Cannot discard a non-draft comment.');
+ }
+ this.disabled = true;
+ var commentID = this.comment.id;
+ if (!commentID) {
+ this.fire('gr-diff-comment-discard');
+ return;
+ }
+ this._send('DELETE', this._restEndpoint(commentID)).then(function(req) {
+ this.fire('gr-diff-comment-discard', this.comment);
+ }.bind(this)).catch(function(err) {
+ alert('Your draft couldn’t be deleted. Check the console and ' +
+ 'contact the PolyGerrit team for assistance.');
+ this.disabled = false;
+ }.bind(this));
+ },
+
+ _send: function(method, url) {
+ var xhr = document.createElement('iron-request');
+ this._xhrPromise = xhr.send({
+ method: method,
+ headers: {
+ 'Content-Type': 'application/json',
+ 'X-Gerrit-Auth': util.getCookie('XSRF_TOKEN'),
+ },
+ url: url,
+ body: this.comment,
+ jsonPrefix: ')]}\'',
+ });
+ return this._xhrPromise;
+ },
+
+ _restEndpoint: function(id) {
+ var path = '/changes/' + this.changeNum + '/revisions/' +
+ this.patchNum + '/drafts';
+ if (id) {
+ path += '/' + id;
+ }
+ return path;
+ },
+
});
})();
</script>
diff --git a/polygerrit-ui/app/elements/gr-diff-view.html b/polygerrit-ui/app/elements/gr-diff-view.html
index 6b2f3f5..81122c3 100644
--- a/polygerrit-ui/app/elements/gr-diff-view.html
+++ b/polygerrit-ui/app/elements/gr-diff-view.html
@@ -81,10 +81,12 @@
.content {
position: relative;
}
- .lineNum.blank {
+ .lineNum.blank,
+ .threadFiller--redLine {
border-right: 2px solid #F34D4D;
margin-right: 3px;
}
+
.lineNum:not(.blank) {
cursor: pointer;
}
@@ -133,6 +135,16 @@
url="[[_computeFilesPath(_changeNum, _patchNum)]]"
json-prefix=")]}'"
on-response="_handleFilesResponse"></iron-ajax>
+ <iron-ajax
+ id="leftDraftsXHR"
+ url="[[_computeDraftsPath(_changeNum, _basePatchNum)]]"
+ json-prefix=")]}'"
+ on-response="_handleLeftDraftsResponse"></iron-ajax>
+ <iron-ajax
+ id="rightDraftsXHR"
+ url="[[_computeDraftsPath(_changeNum, _patchNum)]]"
+ json-prefix=")]}'"
+ on-response="_handleRightDraftsResponse"></iron-ajax>
<h3>
<a href$="[[_computeChangePath(_changeNum)]]">[[_changeNum]]</a><span>:</span>
<span>[[_change.subject]]</span> — <span>[[params.path]]</span>
@@ -184,11 +196,25 @@
type: Array,
value: function() { return []; },
},
- _leftComments: Array,
+ _leftComments: {
+ type: Array,
+ value: function() { return []; },
+ },
+ _leftDrafts: {
+ type: Array,
+ value: function() { return []; },
+ },
_patchNum: String,
_path: String,
_rendered: Boolean,
- _rightComments: Array,
+ _rightComments: {
+ type: Array,
+ value: function() { return []; },
+ },
+ _rightDrafts: {
+ type: Array,
+ value: function() { return []; },
+ },
},
listeners: {
@@ -210,21 +236,48 @@
this._patchNum = null;
this._diff = null;
this._path = null;
- this._leftComments = null;
- this._rightComments = null;
+ this._leftComments = [];
+ this._rightComments = [];
+ this._leftDrafts = [];
+ this._rightDrafts = [];
this._rendered = false;
return;
}
// Assign the params here since a computed binding relying on
// `_basePatchNum` won't fire in the case where it's not defined.
this.$.diffXHR.params = this._diffQueryParams(this._basePatchNum);
- this.$.diffXHR.generateRequest();
+
+ var requestPromises = [];
+ requestPromises.push(this.$.diffXHR.generateRequest().completes);
if (this._basePatchNum) {
- this.$.leftCommentsXHR.generateRequest();
+ requestPromises.push(
+ this.$.leftCommentsXHR.generateRequest().completes);
}
- this.$.rightCommentsXHR.generateRequest();
- this.$.filesXHR.generateRequest();
+
+ requestPromises.push(
+ this.$.rightCommentsXHR.generateRequest().completes);
+ requestPromises.push(this.$.filesXHR.generateRequest().completes);
+
+ app.accountReady.then(function() {
+ if (app.loggedIn) {
+ if (this._basePatchNum) {
+ requestPromises.push(
+ this.$.leftDraftsXHR.generateRequest().completes);
+ }
+ requestPromises.push(
+ this.$.rightDraftsXHR.generateRequest().completes);
+ }
+
+ Promise.all(requestPromises).then(function(requests) {
+ this._renderDiff(this._diff, this._leftComments,
+ this._rightComments, this._leftDrafts, this._rightDrafts);
+ }.bind(this), function(err) {
+ alert('Oops. Something went wrong. Check the console and bug the ' +
+ 'PolyGerrit team for assistance.');
+ throw err;
+ });
+ }.bind(this));
},
_rulerWidthChanged: function(newValue, oldValue) {
@@ -273,6 +326,10 @@
return '/changes/' + changeNum + '/revisions/' + patchNum + '/files';
},
+ _computeDraftsPath: function(changeNum, patchNum) {
+ return '/changes/' + changeNum + '/revisions/' + patchNum + '/drafts';
+ },
+
_diffQueryParams: function(basePatchNum) {
var params = {
context: 'ALL',
@@ -286,21 +343,65 @@
_diffContainerTapHandler: function(e) {
var el = e.detail.sourceEvent.target;
- if (el.classList.contains('lineNum')) {
- // TODO: Implement adding draft comments.
+ // This tap handler only handles line number taps.
+ if (!el.classList.contains('lineNum')) { return; }
+
+ var leftSide = el.parentNode == this.$.leftDiffNumbers;
+ var rightSide = el.parentNode == this.$.rightDiffNumbers;
+ if (leftSide == rightSide) {
+ throw Error('Comment tap event cannot originate from both left and ' +
+ 'right side');
}
+
+ // If a draft or comment is already present at that line, don’t do
+ // anything.
+ var lineNum = el.getAttribute('data-line-num');
+ var patchNum = el.getAttribute('data-patch-num');
+
+ var existingEl = this.$$('gr-diff-comment-thread' +
+ '[data-patch-num="' + patchNum + '"]' +
+ '[data-line-num="' + lineNum + '"]');
+ if (existingEl) {
+ // A comment or draft is already present at this line.
+ return;
+ }
+
+ var tempDraftID = Math.floor(Math.random() * Math.pow(10, 10)) + '';
+ var drafts = [{
+ __draft: true,
+ __draftID: tempDraftID,
+ path: this._path,
+ line: lineNum,
+ }];
+
+ // If the comment is on the left side of a side-by-side diff with the
+ // parent on the left and a patch with patchNum on the right, the patch
+ // number passed to the backend is the right side patchNum when mutating
+ // a draft. The property `side` is used to determine that it should be
+ // on the parent patch, which is inconsistent and why this looks weird.
+ var patchNum = this._patchNum;
+ if (leftSide && this._basePatchNum == null) {
+ drafts[0].side = 'PARENT';
+ patchNum = 'PARENT';
+ }
+
+ this._addThread(drafts, patchNum, lineNum);
},
_handleLeftCommentsResponse: function(e, req) {
this._leftComments = e.detail.response[this._path] || [];
- this._maybeRenderDiff(this._diff, this._leftComments,
- this._rightComments);
},
_handleRightCommentsResponse: function(e, req) {
this._rightComments = e.detail.response[this._path] || [];
- this._maybeRenderDiff(this._diff, this._leftComments,
- this._rightComments);
+ },
+
+ _handleLeftDraftsResponse: function(e, req) {
+ this._leftDrafts = e.detail.response[this._path] || [];
+ },
+
+ _handleRightDraftsResponse: function(e, req) {
+ this._rightDrafts = e.detail.response[this._path] || [];
},
_handleFilesResponse: function(e, req) {
@@ -309,8 +410,6 @@
_handleDiffResponse: function(e, req) {
this._diff = e.detail.response;
- this._maybeRenderDiff(this._diff, this._leftComments,
- this._rightComments);
},
_handleKey: function(e) {
@@ -351,9 +450,17 @@
return 'thread-' + patchNum + '-' + lineNum;
},
- _renderComments: function(comments, patchNum) {
- // Group the comments by line number. Absense of a line number indicates
- // a top-level file comment.
+ _renderCommentsAndDrafts: function(comments, drafts, patchNum) {
+ // Drafts and comments are combined here, with drafts annotated with a
+ // property.
+ var annotatedDrafts = drafts.map(function(d) {
+ d.__draft = true;
+ return d;
+ });
+ comments = comments.concat(annotatedDrafts);
+
+ // Group the comments and drafts by line number. Absence of a line
+ // number indicates a top-level file comment or draft.
var threads = {};
for (var i = 0; i < comments.length; i++) {
@@ -371,8 +478,16 @@
_addThread: function(comments, patchNum, lineNum) {
var el = document.createElement('gr-diff-comment-thread');
el.comments = comments;
+ el.changeNum = this._changeNum;
+ // Assign the element's patchNum to the right side patchNum if the
+ // passed patchNum is 'PARENT' do to the odd behavior of the REST API.
+ // Don't overwrite patchNum since 'PARENT' is used for other properties.
+ el.patchNum = patchNum == 'PARENT' ? this._patchNum : patchNum;
+
var threadID = this._threadID(patchNum, lineNum);
el.setAttribute('data-thread-id', threadID);
+ el.setAttribute('data-line-num', lineNum);
+ el.setAttribute('data-patch-num', patchNum);
// Find the element that the thread should be appended after. In the
// case of a file comment, it will be appended after the first line.
@@ -406,7 +521,7 @@
var els = Polymer.dom(this.root).querySelectorAll(
'[data-row-num="' + rowNum + '"]');
for (var i = 0; i < els.length; i++) {
- // Is this is the side with the comment? Skip if so.
+ // Is this is the column with the comment? Skip if so.
if (els[i].nextSibling &&
els[i].nextSibling.tagName == 'GR-DIFF-COMMENT-THREAD') {
continue;
@@ -414,6 +529,9 @@
var fillerEl = document.createElement('div');
fillerEl.setAttribute('data-thread-id', threadID);
fillerEl.classList.add('js-threadFiller');
+ if (els[i].classList.contains('lineNum')) {
+ fillerEl.classList.add('threadFiller--redLine');
+ }
fillerEl.style.height = e.detail.height + 'px';
Polymer.dom(els[i].parentNode).insertBefore(
fillerEl, els[i].nextSibling);
@@ -426,16 +544,14 @@
}
},
- _maybeRenderDiff: function(diff, leftComments, rightComments) {
+ _renderDiff: function(
+ diff, leftComments, rightComments, leftDrafts, rightDrafts) {
if (this._rendered) {
this._clearChildren(this.$.leftDiffNumbers);
this._clearChildren(this.$.leftDiffContent);
this._clearChildren(this.$.rightDiffNumbers);
this._clearChildren(this.$.rightDiffContent);
}
- if (!diff || !diff.content) { return; }
- if (this._basePatchNum && leftComments == null) { return; }
- if (rightComments == null) { return; }
this.$.diffContainer.classList.toggle('rightOnly',
diff.change_type == Changes.DiffType.ADDED);
@@ -461,10 +577,12 @@
}
if (leftComments) {
- this._renderComments(leftComments, this._basePatchNum);
+ this._renderCommentsAndDrafts(leftComments, leftDrafts,
+ this._basePatchNum);
}
if (rightComments) {
- this._renderComments(rightComments, this._patchNum);
+ this._renderCommentsAndDrafts(rightComments, rightDrafts,
+ this._patchNum);
}
if (this.rulerWidth) {
@@ -561,15 +679,13 @@
el.setAttribute('data-row-num', ctx.rowNum);
});
- var self = this;
- if (this._basePatchNum) {
- [leftLineNumEl, leftColEl].forEach(function(el) {
- el.setAttribute('data-patch-num', self._basePatchNum);
- });
- }
+ [leftLineNumEl, leftColEl].forEach(function(el) {
+ el.setAttribute('data-patch-num', this._basePatchNum || 'PARENT');
+ }.bind(this));
+
[rightLineNumEl, rightColEl].forEach(function(el) {
- el.setAttribute('data-patch-num', self._patchNum);
- });
+ el.setAttribute('data-patch-num', this._patchNum);
+ }.bind(this));
if (ctx.left.content != null) {
leftLineNumEl.textContent = ctx.left.lineNum;
diff --git a/polygerrit-ui/app/scripts/util.js b/polygerrit-ui/app/scripts/util.js
index 2f5ecaf..93a0349 100644
--- a/polygerrit-ui/app/scripts/util.js
+++ b/polygerrit-ui/app/scripts/util.js
@@ -47,3 +47,18 @@
target.tagName == 'BUTTON' ||
target.tagName == 'A';
};
+
+util.getCookie = function(name) {
+ var key = name + '=';
+ var cookies = document.cookie.split(';');
+ for(var i = 0; i < cookies.length; i++) {
+ var c = cookies[i];
+ while (c.charAt(0) == ' ') {
+ c = c.substring(1);
+ }
+ if (c.indexOf(key) == 0) {
+ return c.substring(key.length, c.length);
+ }
+ }
+ return '';
+};
diff --git a/polygerrit-ui/app/test/gr-diff-comment-test.html b/polygerrit-ui/app/test/gr-diff-comment-test.html
index 4c2b8c2..2e7f9bf 100644
--- a/polygerrit-ui/app/test/gr-diff-comment-test.html
+++ b/polygerrit-ui/app/test/gr-diff-comment-test.html
@@ -20,6 +20,8 @@
<script src="../../bower_components/webcomponentsjs/webcomponents-lite.min.js"></script>
<script src="../../bower_components/web-component-tester/browser.js"></script>
+<script src="../../bower_components/page/page.js"></script>
+<script src="../scripts/util.js"></script>
<link rel="import" href="../../bower_components/iron-test-helpers/iron-test-helpers.html">
<link rel="import" href="../elements/gr-diff-comment.html">
@@ -30,11 +32,27 @@
</template>
</test-fixture>
+<test-fixture id="draft">
+ <template>
+ <gr-diff-comment draft="true"></gr-diff-comment>
+ </template>
+</test-fixture>
+
<script>
suite('gr-diff-comment tests', function() {
var element;
setup(function() {
element = fixture('basic');
+ element.comment = {
+ 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',
+ }
});
test('proper event fires on reply', function(done) {
@@ -51,5 +69,185 @@
MockInteractions.tap(element.$$('.done'));
});
+ test('clicking on date link does not trigger nav', function() {
+ var showStub = sinon.stub(page, 'show');
+ var dateEl = element.$$('.date');
+ assert.ok(dateEl);
+ MockInteractions.tap(dateEl);
+ var dest = window.location.pathname + '#5';
+ assert(showStub.lastCall.calledWithExactly(dest, null, false),
+ 'Should navigate to ' + dest + ' without triggering nav');
+ showStub.restore();
+ });
+ });
+
+ suite('gr-diff-comment draft tests', function() {
+ var element;
+ var server;
+
+ setup(function() {
+ element = fixture('draft');
+ element.changeNum = 42;
+ element.patchNum = 1;
+ element.comment = {
+ __draft: true,
+ __draftID: 'temp_draft_id',
+ path: '/path/to/file',
+ line: 5,
+ };
+
+ server = sinon.fakeServer.create();
+ server.respondWith(
+ 'PUT',
+ '/changes/42/revisions/1/drafts',
+ [
+ 201,
+ { 'Content-Type': 'application/json' },
+ ')]}\'\n{' +
+ '"id": "baf0414d_40572e03",' +
+ '"path": "/path/to/file",' +
+ '"line": 5,' +
+ '"updated": "2015-12-08 21:52:36.177000000",' +
+ '"message": "created!"' +
+ '}'
+ ]
+ );
+
+ server.respondWith(
+ 'PUT',
+ /\/changes\/42\/revisions\/1\/drafts\/.+/,
+ [
+ 200,
+ { 'Content-Type': 'application/json' },
+ ')]}\'\n{' +
+ '"id": "baf0414d_40572e03",' +
+ '"path": "/path/to/file",' +
+ '"line": 5,' +
+ '"updated": "2015-12-08 21:52:36.177000000",' +
+ '"message": "saved!"' +
+ '}'
+ ]
+ );
+ });
+
+ teardown(function() {
+ server.restore();
+ });
+
+ function isVisible(el) {
+ assert.ok(el);
+ return getComputedStyle(el).getPropertyValue('display') != 'none';
+ }
+
+ test('button visibility states', function() {
+ element.draft = true;
+ assert.isTrue(isVisible(element.$$('.edit')), 'edit is visible');
+ 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.$$('.done')), 'done is not visible');
+
+ element.editing = true;
+ assert.isFalse(isVisible(element.$$('.edit')), 'edit is not visible')
+ assert.isTrue(isVisible(element.$$('.discard')), 'discard is visible');
+ assert.isTrue(isVisible(element.$$('.save')), 'save is visible');
+ assert.isTrue(isVisible(element.$$('.cancel')), 'cancel is visible');
+ assert.isFalse(isVisible(element.$$('.reply')), 'reply is not visible');
+ assert.isFalse(isVisible(element.$$('.done')), 'done is not visible');
+
+ element.draft = false,
+ element.editing = false;
+ assert.isFalse(isVisible(element.$$('.edit')), 'edit is not visible')
+ assert.isFalse(isVisible(element.$$('.discard')),
+ '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')), 'edit is visible');
+ assert.isTrue(isVisible(element.$$('.done')), 'edit is visible');
+
+ element.draft = true;
+ });
+
+ test('draft creation/cancelation', function(done) {
+ assert.isFalse(element.editing);
+ MockInteractions.tap(element.$$('.edit'));
+ assert.isTrue(element.editing);
+
+ element._editDraft = '';
+ // Save should be disabled on an empty message.
+ var disabled = element.$$('.save').hasAttribute('disabled');
+ assert.isTrue(disabled, 'save button should be disabled.');
+ element._editDraft == ' ';
+ disabled = element.$$('.save').hasAttribute('disabled');
+ assert.isTrue(disabled, 'save button should be disabled.');
+
+ var numDiscardEvents = 0;
+ element.addEventListener('gr-diff-comment-discard', function(e) {
+ numDiscardEvents++;
+ if (numDiscardEvents == 2) {
+ done();
+ }
+ });
+ MockInteractions.tap(element.$$('.cancel'));
+ MockInteractions.tap(element.$$('.discard'));
+ });
+
+ test('draft saving/editing', function(done) {
+ element.draft = true;
+ MockInteractions.tap(element.$$('.edit'));
+ element._editDraft = 'good news, everyone!';
+ MockInteractions.tap(element.$$('.save'));
+ assert.isTrue(element.disabled,
+ 'Element should be disabled when creating draft.');
+
+ server.respond();
+
+ element._xhrPromise.then(function(req) {
+ assert.isFalse(element.disabled,
+ 'Element should be enabled when done creating draft.');
+ assert.equal(req.status, 201);
+ assert.equal(req.url, '/changes/42/revisions/1/drafts');
+ assert.equal(req.response.message, 'created!');
+ assert.isFalse(element.editing);
+ }).then(function() {
+ MockInteractions.tap(element.$$('.edit'));
+ element._editDraft = 'You’ll be delivering a package to Chapek 9, a ' +
+ 'world where humans are killed on sight.';
+ MockInteractions.tap(element.$$('.save'));
+ assert.isTrue(element.disabled,
+ 'Element should be disabled when updating draft.');
+ server.respond();
+
+ element._xhrPromise.then(function(req) {
+ assert.isFalse(element.disabled,
+ 'Element should be enabled when done updating draft.');
+ assert.equal(req.status, 200);
+ assert.equal(req.url,
+ '/changes/42/revisions/1/drafts/baf0414d_40572e03');
+ assert.equal(req.response.message, 'saved!');
+ assert.isFalse(element.editing);
+ done();
+ });
+ });
+ });
+
+ test('proper event fires on done', function(done) {
+ element.addEventListener('gr-diff-comment-done', 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');
+ assert.ok(dateEl);
+ MockInteractions.tap(dateEl);
+ var dest = window.location.pathname + '#5';
+ assert(showStub.lastCall.calledWithExactly(dest, null, false),
+ 'Should navigate to ' + dest + ' without triggering nav');
+ showStub.restore();
+ });
});
</script>
diff --git a/polygerrit-ui/app/test/gr-diff-view-test.html b/polygerrit-ui/app/test/gr-diff-view-test.html
index 09472a3..8a6ac3b 100644
--- a/polygerrit-ui/app/test/gr-diff-view-test.html
+++ b/polygerrit-ui/app/test/gr-diff-view-test.html
@@ -45,7 +45,6 @@
</template>
</test-fixture>
-
<script>
// Original diff:
// Left side (side A):
@@ -101,7 +100,8 @@
setup(function() {
element = fixture('basic');
- element._maybeRenderDiff({content: diffContent}, [], []);
+ element._renderDiff({content: diffContent}, [], [], [], []);
+ flushAsynchronousOperations();
});
test('ab content is the same for left and right sides', function() {
@@ -174,62 +174,79 @@
assert.equal(els[i].style.left, '100ch');
}
});
-
});
- suite('comments', function() {
+ suite('comments and drafts', function() {
var element;
- setup(function() {
+ setup(function(done) {
element = fixture('comments');
element._patchNum = 1;
- element._maybeRenderDiff({content: diffContent}, [], [
- {
- id: 'file_comment',
- message: 'this is a file comment about the meaninglessness of life',
- author: {
- name: 'GLaDOS'
- }
- },
- {
- id: 'all_the_lemons',
- line: 8,
- message: 'MAKE LIFE TAKE THE LEMONS BACK',
- author: {
- name: 'Cave Johnson',
+ element._renderDiff({content: diffContent}, [], [
+ {
+ id: 'file_comment',
+ message: 'this is a file comment about the meaninglessness of life',
+ author: {
+ name: 'GLaDOS'
+ }
},
- }
- ]);
+ {
+ id: 'all_the_lemons',
+ line: 8,
+ message: 'MAKE LIFE TAKE THE LEMONS BACK',
+ author: {
+ name: 'Cave Johnson',
+ }
+ }
+ ], [], []);
+
+ // On WebKit and Gecko, flushAsynchronousOperations isn't enough to allow
+ // the thread filler elements to properly render. Spin the runloop.
+ element.async(function() {
+ done();
+ }, 1);
});
- test('comment threads are rendered correctly', function(done) {
- // On WebKit and Gecko, flushAsynchronousOperations isn't enough to allow
- // the thread filler elements to properly render. Wait for the resize
- // events that trigger their addition and check after the expected number
- // come in.
- var numEventsFired = 0;
- element.addEventListener('gr-diff-comment-thread-height-changed',
- function() {
- numEventsFired++;
- if (numEventsFired < 2) { return; }
- assert.equal(numEventsFired, 2);
+ test('comment threads are rendered correctly', function() {
+ var threadEls = Polymer.dom(element.root).querySelectorAll(
+ 'gr-diff-comment-thread[data-thread-id="thread-1-8"]');
+ assert.equal(threadEls.length, 1);
+ var fillerEls = Polymer.dom(element.root).querySelectorAll(
+ '.js-threadFiller[data-thread-id="thread-1-8"]');
+ assert.equal(fillerEls.length, 3);
- var threadEls = Polymer.dom(element.root).querySelectorAll(
- 'gr-diff-comment-thread[data-thread-id="thread-1-8"]');
- assert.equal(threadEls.length, 1);
- var fillerEls = Polymer.dom(element.root).querySelectorAll(
- '.js-threadFiller[data-thread-id="thread-1-8"]');
- assert.equal(fillerEls.length, 3);
+ threadEls = Polymer.dom(element.root).querySelectorAll(
+ 'gr-diff-comment-thread[data-thread-id="thread-1-FILE"]');
+ assert.equal(threadEls.length, 1);
+ fillerEls = Polymer.dom(element.root).querySelectorAll(
+ '.js-threadFiller[data-thread-id="thread-1-FILE"]');
+ assert.equal(fillerEls.length, 3);
+ });
- threadEls = Polymer.dom(element.root).querySelectorAll(
- 'gr-diff-comment-thread[data-thread-id="thread-1-FILE"]');
- assert.equal(threadEls.length, 1);
- fillerEls = Polymer.dom(element.root).querySelectorAll(
- '.js-threadFiller[data-thread-id="thread-1-FILE"]');
- assert.equal(fillerEls.length, 3);
+ test('tapping a line with an existing thread', function() {
+ var threadEls = Polymer.dom(element.root).querySelectorAll(
+ 'gr-diff-comment-thread[data-line-num="8"][data-patch-num="1"]');
+ assert.equal(threadEls.length, 1);
+ var lineEl = element.$$(
+ '.lineNum[data-line-num="8"][data-patch-num="1"]');
+ assert.ok(lineEl);
+ MockInteractions.tap(lineEl);
+ threadEls = Polymer.dom(element.root).querySelectorAll(
+ 'gr-diff-comment-thread[data-line-num="8"][data-patch-num="1"]');
+ assert.equal(threadEls.length, 1);
+ });
- done();
- });
+ test('creating a draft', function() {
+ var threadEls = Polymer.dom(element.root).querySelectorAll(
+ 'gr-diff-comment-thread[data-line-num="5"][data-patch-num="1"]');
+ assert.equal(threadEls.length, 0);
+ var lineEl = element.$$(
+ '.lineNum[data-line-num="5"][data-patch-num="1"]');
+ assert.ok(lineEl);
+ MockInteractions.tap(lineEl);
+ threadEls = Polymer.dom(element.root).querySelectorAll(
+ 'gr-diff-comment-thread[data-line-num="5"][data-patch-num="1"]');
+ assert.equal(threadEls.length, 1);
});
});
@@ -242,7 +259,7 @@
for (var i = 0; i < 300; i++) {
longDiffContent[0].ab.push('');
}
- element._maybeRenderDiff({content: longDiffContent}, [], []);
+ element._renderDiff({content: longDiffContent}, [], [], [], []);
});
function isVisibleInWindow(el) {
