diff --git a/lib/js/BUCK b/lib/js/BUCK index d03753e48f..060788c2fa 100644 --- a/lib/js/BUCK +++ b/lib/js/BUCK @@ -104,6 +104,21 @@ bower_component( sha1 = 'f94a3a3d847842c49def41e27da42c7c94f8d7c7', ) +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', @@ -150,6 +165,15 @@ bower_component( sha1 = '3ca2fbbf3b56d95677663f78304262dee68753c3', ) +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', @@ -206,7 +230,6 @@ bower_component( 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 af8e68347c..73b52212fc 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 af42cab295..15b11f5dda 100644 --- a/polygerrit-ui/BUCK +++ b/polygerrit-ui/BUCK @@ -5,6 +5,7 @@ bower_components( 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 b908cf78d4..54549144c3 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 @@ limitations under the License. } diff --git a/polygerrit-ui/app/elements/gr-diff-comment.html b/polygerrit-ui/app/elements/gr-diff-comment.html index 343f7d3bf9..f1379962e7 100644 --- a/polygerrit-ui/app/elements/gr-diff-comment.html +++ b/polygerrit-ui/app/elements/gr-diff-comment.html @@ -15,6 +15,9 @@ limitations under the License. --> + + + @@ -24,6 +27,12 @@ limitations under the License. border: 1px solid #ddd; display: block; } + :host([disabled]) { + pointer-events: none; + } + :host([disabled]) .container { + opacity: .5; + } .header, .message, .actions { @@ -31,32 +40,113 @@ limitations under the License. } .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; + } - -
[[comment.message]]
-
- Reply - Done +
+ + +
[[comment.message]]
+
+ Reply + Done + Edit + Save + Cancel +
+ Discard +
+
diff --git a/polygerrit-ui/app/elements/gr-diff-view.html b/polygerrit-ui/app/elements/gr-diff-view.html index 6b2f3f5498..81122c3b00 100644 --- a/polygerrit-ui/app/elements/gr-diff-view.html +++ b/polygerrit-ui/app/elements/gr-diff-view.html @@ -81,10 +81,12 @@ limitations under the License. .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 @@ limitations under the License. url="[[_computeFilesPath(_changeNum, _patchNum)]]" json-prefix=")]}'" on-response="_handleFilesResponse"> + +

[[_changeNum]]: [[_change.subject]] — [[params.path]] @@ -184,11 +196,25 @@ limitations under the License. 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 @@ limitations under the License. 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 @@ limitations under the License. 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 @@ limitations under the License. _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 @@ limitations under the License. _handleDiffResponse: function(e, req) { this._diff = e.detail.response; - this._maybeRenderDiff(this._diff, this._leftComments, - this._rightComments); }, _handleKey: function(e) { @@ -351,9 +450,17 @@ limitations under the License. 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 @@ limitations under the License. _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 @@ limitations under the License. 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 @@ limitations under the License. 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 @@ limitations under the License. } }, - _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 @@ limitations under the License. } 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 @@ limitations under the License. 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 2f5ecafd05..93a0349848 100644 --- a/polygerrit-ui/app/scripts/util.js +++ b/polygerrit-ui/app/scripts/util.js @@ -47,3 +47,18 @@ util.shouldSupressKeyboardShortcut = function(e) { 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 4c2b8c257f..2e7f9bfe7d 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 @@ limitations under the License. + + @@ -30,11 +32,27 @@ limitations under the License. + + + + diff --git a/polygerrit-ui/app/test/gr-diff-view-test.html b/polygerrit-ui/app/test/gr-diff-view-test.html index 09472a3216..8a6ac3bb4b 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 @@ limitations under the License. -