diff options
author | Jacob Schatz <jschatz@gitlab.com> | 2017-03-08 16:07:27 +0000 |
---|---|---|
committer | DJ Mountney <david@twkie.net> | 2017-03-10 13:04:06 -0800 |
commit | c5c718ef43b5b42669942ad6c374603a5e119d03 (patch) | |
tree | 36bb69304d2e9a16d3104cc763a6a8b00520ee3c | |
parent | bd1d778168513e3889df877911589a371277e076 (diff) | |
download | gitlab-ce-c5c718ef43b5b42669942ad6c374603a5e119d03.tar.gz |
Merge branch 'diff-comments-avatars' into 'master'
Added discussion comments avatars to diff
Closes #17237
See merge request !7357
18 files changed, 566 insertions, 52 deletions
diff --git a/app/assets/javascripts/diff_notes/components/diff_note_avatars.js b/app/assets/javascripts/diff_notes/components/diff_note_avatars.js new file mode 100644 index 00000000000..788daa96b3d --- /dev/null +++ b/app/assets/javascripts/diff_notes/components/diff_note_avatars.js @@ -0,0 +1,155 @@ +/* global CommentsStore Cookies notes */ +import Vue from 'vue'; +import collapseIcon from '../icons/collapse_icon.svg'; + +(() => { + const DiffNoteAvatars = Vue.extend({ + props: ['discussionId'], + data() { + return { + isVisible: false, + lineType: '', + storeState: CommentsStore.state, + shownAvatars: 3, + collapseIcon, + }; + }, + template: ` + <div class="diff-comment-avatar-holders" + v-show="notesCount !== 0"> + <div v-if="!isVisible"> + <img v-for="note in notesSubset" + class="avatar diff-comment-avatar has-tooltip js-diff-comment-avatar" + width="19" + height="19" + role="button" + data-container="body" + data-placement="top" + :data-line-type="lineType" + :title="note.authorName + ': ' + note.noteTruncated" + :src="note.authorAvatar" + @click="clickedAvatar($event)" /> + <span v-if="notesCount > shownAvatars" + class="diff-comments-more-count has-tooltip js-diff-comment-avatar" + data-container="body" + data-placement="top" + ref="extraComments" + role="button" + :data-line-type="lineType" + :title="extraNotesTitle" + @click="clickedAvatar($event)">{{ moreText }}</span> + </div> + <button class="diff-notes-collapse js-diff-comment-avatar" + type="button" + aria-label="Show comments" + :data-line-type="lineType" + @click="clickedAvatar($event)" + v-if="isVisible" + v-html="collapseIcon"> + </button> + </div> + `, + mounted() { + this.$nextTick(() => { + this.addNoCommentClass(); + this.setDiscussionVisible(); + + this.lineType = $(this.$el).closest('.diff-line-num').hasClass('old_line') ? 'old' : 'new'; + }); + + $(document).on('toggle.comments', () => { + this.$nextTick(() => { + this.setDiscussionVisible(); + }); + }); + }, + destroyed() { + $(document).off('toggle.comments'); + }, + watch: { + storeState: { + handler() { + this.$nextTick(() => { + $('.has-tooltip', this.$el).tooltip('fixTitle'); + + // We need to add/remove a class to an element that is outside the Vue instance + this.addNoCommentClass(); + }); + }, + deep: true, + }, + }, + computed: { + notesSubset() { + let notes = []; + + if (this.discussion) { + notes = Object.keys(this.discussion.notes) + .slice(0, this.shownAvatars) + .map(noteId => this.discussion.notes[noteId]); + } + + return notes; + }, + extraNotesTitle() { + if (this.discussion) { + const extra = this.discussion.notesCount() - this.shownAvatars; + + return `${extra} more comment${extra > 1 ? 's' : ''}`; + } + + return ''; + }, + discussion() { + return this.storeState[this.discussionId]; + }, + notesCount() { + if (this.discussion) { + return this.discussion.notesCount(); + } + + return 0; + }, + moreText() { + const plusSign = this.notesCount < 100 ? '+' : ''; + + return `${plusSign}${this.notesCount - this.shownAvatars}`; + }, + }, + methods: { + clickedAvatar(e) { + notes.addDiffNote(e); + + // Toggle the active state of the toggle all button + this.toggleDiscussionsToggleState(); + + this.$nextTick(() => { + this.setDiscussionVisible(); + + $('.has-tooltip', this.$el).tooltip('fixTitle'); + $('.has-tooltip', this.$el).tooltip('hide'); + }); + }, + addNoCommentClass() { + const notesCount = this.notesCount; + + $(this.$el).closest('.js-avatar-container') + .toggleClass('js-no-comment-btn', notesCount > 0) + .nextUntil('.js-avatar-container') + .toggleClass('js-no-comment-btn', notesCount > 0); + }, + toggleDiscussionsToggleState() { + const $notesHolders = $(this.$el).closest('.code').find('.notes_holder'); + const $visibleNotesHolders = $notesHolders.filter(':visible'); + const $toggleDiffCommentsBtn = $(this.$el).closest('.diff-file').find('.js-toggle-diff-comments'); + + $toggleDiffCommentsBtn.toggleClass('active', $notesHolders.length === $visibleNotesHolders.length); + }, + setDiscussionVisible() { + this.isVisible = $(`.diffs .notes[data-discussion-id="${this.discussion.id}"]`).is(':visible'); + }, + }, + }); + + Vue.component('diff-note-avatars', DiffNoteAvatars); +})(); diff --git a/app/assets/javascripts/diff_notes/components/resolve_btn.js b/app/assets/javascripts/diff_notes/components/resolve_btn.js index d1873d6c7a2..fbd980f0fce 100644 --- a/app/assets/javascripts/diff_notes/components/resolve_btn.js +++ b/app/assets/javascripts/diff_notes/components/resolve_btn.js @@ -11,7 +11,10 @@ const Vue = require('vue'); discussionId: String, resolved: Boolean, canResolve: Boolean, - resolvedBy: String + resolvedBy: String, + authorName: String, + authorAvatar: String, + noteTruncated: String, }, data: function () { return { @@ -98,7 +101,16 @@ const Vue = require('vue'); CommentsStore.delete(this.discussionId, this.noteId); }, created: function () { - CommentsStore.create(this.discussionId, this.noteId, this.canResolve, this.resolved, this.resolvedBy); + CommentsStore.create({ + discussionId: this.discussionId, + noteId: this.noteId, + canResolve: this.canResolve, + resolved: this.resolved, + resolvedBy: this.resolvedBy, + authorName: this.authorName, + authorAvatar: this.authorAvatar, + noteTruncated: this.noteTruncated, + }); this.note = this.discussion.getNote(this.noteId); } diff --git a/app/assets/javascripts/diff_notes/diff_notes_bundle.js b/app/assets/javascripts/diff_notes/diff_notes_bundle.js index cadf8b96b87..7d8316dfd63 100644 --- a/app/assets/javascripts/diff_notes/diff_notes_bundle.js +++ b/app/assets/javascripts/diff_notes/diff_notes_bundle.js @@ -13,6 +13,7 @@ require('./components/jump_to_discussion'); require('./components/resolve_btn'); require('./components/resolve_count'); require('./components/resolve_discussion_btn'); +require('./components/diff_note_avatars'); $(() => { const projectPath = document.querySelector('.merge-request').dataset.projectPath; @@ -24,6 +25,15 @@ $(() => { window.ResolveService = new gl.DiffNotesResolveServiceClass(projectPath); gl.diffNotesCompileComponents = () => { + $('diff-note-avatars').each(function () { + const tmp = Vue.extend({ + template: $(this).get(0).outerHTML + }); + const tmpApp = new tmp().$mount(); + + $(this).replaceWith(tmpApp.$el); + }); + const $components = $(COMPONENT_SELECTOR).filter(function () { return $(this).closest('resolve-count').length !== 1; }); diff --git a/app/assets/javascripts/diff_notes/icons/collapse_icon.svg b/app/assets/javascripts/diff_notes/icons/collapse_icon.svg new file mode 100644 index 00000000000..bd4b393cfaa --- /dev/null +++ b/app/assets/javascripts/diff_notes/icons/collapse_icon.svg @@ -0,0 +1 @@ +<svg width="11" height="11" viewBox="0 0 9 13"><path d="M2.57568253,6.49866948 C2.50548852,6.57199715 2.44637866,6.59708255 2.39835118,6.57392645 C2.3503237,6.55077034 2.32631032,6.48902165 2.32631032,6.38867852 L2.32631032,-2.13272614 C2.32631032,-2.23306927 2.3503237,-2.29481796 2.39835118,-2.31797406 C2.44637866,-2.34113017 2.50548852,-2.31604477 2.57568253,-2.24271709 L6.51022184,1.86747129 C6.53977721,1.8983461 6.56379059,1.93500939 6.5822627,1.97746225 L6.5822627,2.27849013 C6.56379059,2.31708364 6.53977721,2.35374693 6.51022184,2.38848109 L2.57568253,6.49866948 Z" transform="translate(4.454287, 2.127976) rotate(90.000000) translate(-4.454287, -2.127976) "></path><path d="M3.74312342,2.09553332 C3.74312342,1.99519019 3.77821989,1.9083561 3.8484139,1.83502843 C3.91860791,1.76170075 4.00173115,1.72503747 4.09778611,1.72503747 L4.80711151,1.72503747 C4.90316647,1.72503747 4.98628971,1.76170075 5.05648372,1.83502843 C5.12667773,1.9083561 5.16177421,1.99519019 5.16177421,2.09553332 L5.16177421,10.2464421 C5.16177421,10.3467853 5.12667773,10.4336194 5.05648372,10.506947 C4.98628971,10.5802747 4.90316647,10.616938 4.80711151,10.616938 L4.09778611,10.616938 C4.00173115,10.616938 3.91860791,10.5802747 3.8484139,10.506947 C3.77821989,10.4336194 3.74312342,10.3467853 3.74312342,10.2464421 L3.74312342,2.09553332 Z" transform="translate(4.452449, 6.170988) rotate(-90.000000) translate(-4.452449, -6.170988) "></path><path d="M2.57568253,14.6236695 C2.50548852,14.6969971 2.44637866,14.7220826 2.39835118,14.6989264 C2.3503237,14.6757703 2.32631032,14.6140216 2.32631032,14.5136785 L2.32631032,5.99227386 C2.32631032,5.89193073 2.3503237,5.83018204 2.39835118,5.80702594 C2.44637866,5.78386983 2.50548852,5.80895523 2.57568253,5.88228291 L6.51022184,9.99247129 C6.53977721,10.0233461 6.56379059,10.0600094 6.5822627,10.1024622 L6.5822627,10.4034901 C6.56379059,10.4420836 6.53977721,10.4787469 6.51022184,10.5134811 L2.57568253,14.6236695 Z" transform="translate(4.454287, 10.252976) scale(1, -1) rotate(90.000000) translate(-4.454287, -10.252976) "></path></svg> diff --git a/app/assets/javascripts/diff_notes/models/discussion.js b/app/assets/javascripts/diff_notes/models/discussion.js index fa518ba4d33..dce1a9b58bd 100644 --- a/app/assets/javascripts/diff_notes/models/discussion.js +++ b/app/assets/javascripts/diff_notes/models/discussion.js @@ -10,8 +10,8 @@ class DiscussionModel { this.canResolve = false; } - createNote (noteId, canResolve, resolved, resolved_by) { - Vue.set(this.notes, noteId, new NoteModel(this.id, noteId, canResolve, resolved, resolved_by)); + createNote (noteObj) { + Vue.set(this.notes, noteObj.noteId, new NoteModel(this.id, noteObj)); } deleteNote (noteId) { diff --git a/app/assets/javascripts/diff_notes/models/note.js b/app/assets/javascripts/diff_notes/models/note.js index f3a7cba5ef6..04465aa507e 100644 --- a/app/assets/javascripts/diff_notes/models/note.js +++ b/app/assets/javascripts/diff_notes/models/note.js @@ -1,12 +1,15 @@ /* eslint-disable camelcase, no-unused-vars */ class NoteModel { - constructor(discussionId, noteId, canResolve, resolved, resolved_by) { + constructor(discussionId, noteObj) { this.discussionId = discussionId; - this.id = noteId; - this.canResolve = canResolve; - this.resolved = resolved; - this.resolved_by = resolved_by; + this.id = noteObj.noteId; + this.canResolve = noteObj.canResolve; + this.resolved = noteObj.resolved; + this.resolved_by = noteObj.resolvedBy; + this.authorName = noteObj.authorName; + this.authorAvatar = noteObj.authorAvatar; + this.noteTruncated = noteObj.noteTruncated; } } diff --git a/app/assets/javascripts/diff_notes/stores/comments.js b/app/assets/javascripts/diff_notes/stores/comments.js index c80d979b977..69c4d7a8434 100644 --- a/app/assets/javascripts/diff_notes/stores/comments.js +++ b/app/assets/javascripts/diff_notes/stores/comments.js @@ -21,10 +21,10 @@ return discussion; }, - create: function (discussionId, noteId, canResolve, resolved, resolved_by) { - const discussion = this.createDiscussion(discussionId); + create: function (noteObj) { + const discussion = this.createDiscussion(noteObj.discussionId); - discussion.createNote(noteId, canResolve, resolved, resolved_by); + discussion.createNote(noteObj); }, update: function (discussionId, noteId, resolved, resolved_by) { const discussion = this.state[discussionId]; diff --git a/app/assets/javascripts/files_comment_button.js b/app/assets/javascripts/files_comment_button.js index 6d86888dcb8..bf84f2a0a8f 100644 --- a/app/assets/javascripts/files_comment_button.js +++ b/app/assets/javascripts/files_comment_button.js @@ -38,6 +38,9 @@ FilesCommentButton.prototype.render = function(e) { var $currentTarget, buttonParentElement, lineContentElement, textFileElement, $button; $currentTarget = $(e.currentTarget); + + if ($currentTarget.hasClass('js-no-comment-btn')) return; + lineContentElement = this.getLineContent($currentTarget); buttonParentElement = this.getButtonParent($currentTarget); diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index ae4dd64424c..79164edff0e 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -342,11 +342,11 @@ require('./zen_mode'); var notesHolders = $this.closest('.diff-file').find('.notes_holder'); $this.toggleClass('active'); if ($this.hasClass('active')) { - notesHolders.show().find('.hide').show(); + notesHolders.show().find('.hide, .content').show(); } else { - notesHolders.hide(); + notesHolders.hide().find('.content').hide(); } - $this.trigger('blur'); + $(document).trigger('toggle.comments'); return e.preventDefault(); }); $document.off('click', '.js-confirm-danger'); diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index df7a7d2a459..eeab69da941 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -312,7 +312,7 @@ require('./task_list'); */ Notes.prototype.renderDiscussionNote = function(note) { - var discussionContainer, form, note_html, row; + var discussionContainer, form, note_html, row, lineType, diffAvatarContainer; if (!this.isNewNote(note)) { return; } @@ -322,6 +322,8 @@ require('./task_list'); form = $("#new-discussion-note-form-" + note.original_discussion_id); } row = form.closest("tr"); + lineType = this.isParallelView() ? form.find('#line_type').val() : 'old'; + diffAvatarContainer = row.prevAll('.line_holder').first().find('.js-avatar-container.' + lineType + '_line'); note_html = $(note.html); note_html.renderGFM(); // is this the first note of discussion? @@ -330,10 +332,26 @@ require('./task_list'); discussionContainer = $(".notes[data-discussion-id='" + note.original_discussion_id + "']"); } if (discussionContainer.length === 0) { - // insert the note and the reply button after the temp row - row.after(note.diff_discussion_html); - // remove the note (will be added again below) - row.next().find(".note").remove(); + if (!this.isParallelView() || row.hasClass('js-temp-notes-holder')) { + // insert the note and the reply button after the temp row + row.after(note.diff_discussion_html); + + // remove the note (will be added again below) + row.next().find(".note").remove(); + } else { + // Merge new discussion HTML in + var $discussion = $(note.diff_discussion_html); + var $notes = $discussion.find('.notes[data-discussion-id="' + note.discussion_id + '"]'); + var contentContainerClass = '.' + $notes.closest('.notes_content') + .attr('class') + .split(' ') + .join('.'); + + // remove the note (will be added again below) + $notes.find('.note').remove(); + + row.find(contentContainerClass + ' .content').append($notes.closest('.content').children()); + } // Before that, the container didn't exist discussionContainer = $(".notes[data-discussion-id='" + note.discussion_id + "']"); // Add note to 'Changes' page discussions @@ -347,14 +365,40 @@ require('./task_list'); discussionContainer.append(note_html); } - if (typeof gl.diffNotesCompileComponents !== 'undefined') { + if (typeof gl.diffNotesCompileComponents !== 'undefined' && note.discussion_id) { gl.diffNotesCompileComponents(); + this.renderDiscussionAvatar(diffAvatarContainer, note); } gl.utils.localTimeAgo($('.js-timeago'), false); return this.updateNotesCount(1); }; + Notes.prototype.getLineHolder = function(changesDiscussionContainer) { + return $(changesDiscussionContainer).closest('.notes_holder') + .prevAll('.line_holder') + .first() + .get(0); + }; + + Notes.prototype.renderDiscussionAvatar = function(diffAvatarContainer, note) { + var commentButton = diffAvatarContainer.find('.js-add-diff-note-button'); + var avatarHolder = diffAvatarContainer.find('.diff-comment-avatar-holders'); + + if (!avatarHolder.length) { + avatarHolder = document.createElement('diff-note-avatars'); + avatarHolder.setAttribute('discussion-id', note.discussion_id); + + diffAvatarContainer.append(avatarHolder); + + gl.diffNotesCompileComponents(); + } + + if (commentButton.length) { + commentButton.remove(); + } + }; + /* Called in response the main target form has been successfully submitted. @@ -592,9 +636,14 @@ require('./task_list'); */ Notes.prototype.removeNote = function(e) { - var noteId; - noteId = $(e.currentTarget).closest(".note").attr("id"); - $(".note[id='" + noteId + "']").each((function(_this) { + var noteElId, noteId, dataNoteId, $note, lineHolder; + $note = $(e.currentTarget).closest('.note'); + noteElId = $note.attr('id'); + noteId = $note.attr('data-note-id'); + lineHolder = $(e.currentTarget).closest('.notes[data-discussion-id]') + .closest('.notes_holder') + .prev('.line_holder'); + $(".note[id='" + noteElId + "']").each((function(_this) { // A same note appears in the "Discussion" and in the "Changes" tab, we have // to remove all. Using $(".note[id='noteId']") ensure we get all the notes, // where $("#noteId") would return only one. @@ -604,17 +653,26 @@ require('./task_list'); notes = note.closest(".notes"); if (typeof gl.diffNotesCompileComponents !== 'undefined') { - if (gl.diffNoteApps[noteId]) { - gl.diffNoteApps[noteId].$destroy(); + if (gl.diffNoteApps[noteElId]) { + gl.diffNoteApps[noteElId].$destroy(); } } + note.remove(); + // check if this is the last note for this line - if (notes.find(".note").length === 1) { + if (notes.find(".note").length === 0) { + var notesTr = notes.closest("tr"); + // "Discussions" tab notes.closest(".timeline-entry").remove(); - // "Changes" tab / commit view - notes.closest("tr").remove(); + + if (!_this.isParallelView() || notesTr.find('.note').length === 0) { + // "Changes" tab / commit view + notesTr.remove(); + } else { + notes.closest('.content').empty(); + } } return note.remove(); }; @@ -707,15 +765,16 @@ require('./task_list'); */ Notes.prototype.addDiffNote = function(e) { - var $link, addForm, hasNotes, lineType, newForm, nextRow, noteForm, notesContent, notesContentSelector, replyButton, row, rowCssToAdd, targetContent; + var $link, addForm, hasNotes, lineType, newForm, nextRow, noteForm, notesContent, notesContentSelector, replyButton, row, rowCssToAdd, targetContent, isDiffCommentAvatar; e.preventDefault(); - $link = $(e.currentTarget); + $link = $(e.currentTarget || e.target); row = $link.closest("tr"); nextRow = row.next(); hasNotes = nextRow.is(".notes_holder"); addForm = false; notesContentSelector = ".notes_content"; rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\" colspan=\"2\"></td><td class=\"notes_content\"><div class=\"content\"></div></td></tr>"; + isDiffCommentAvatar = $link.hasClass('js-diff-comment-avatar'); // In parallel view, look inside the correct left/right pane if (this.isParallelView()) { lineType = $link.data("lineType"); @@ -723,7 +782,9 @@ require('./task_list'); rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line old\"></td><td class=\"notes_content parallel old\"><div class=\"content\"></div></td><td class=\"notes_line new\"></td><td class=\"notes_content parallel new\"><div class=\"content\"></div></td></tr>"; } notesContentSelector += " .content"; - if (hasNotes) { + notesContent = nextRow.find(notesContentSelector); + + if (hasNotes && !isDiffCommentAvatar) { nextRow.show(); notesContent = nextRow.find(notesContentSelector); if (notesContent.length) { @@ -740,13 +801,21 @@ require('./task_list'); } } } - } else { + } else if (!isDiffCommentAvatar) { // add a notes row and insert the form row.after(rowCssToAdd); nextRow = row.next(); notesContent = nextRow.find(notesContentSelector); addForm = true; + } else { + nextRow.show(); + notesContent.toggle(!notesContent.is(':visible')); + + if (!nextRow.find('.content:not(:empty)').is(':visible')) { + nextRow.hide(); + } } + if (addForm) { newForm = this.formClone.clone(); newForm.appendTo(notesContent); diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 5d0c247dea8..eab79c2a481 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -113,6 +113,10 @@ td.line_content.parallel { width: 46%; } + + .add-diff-note { + margin-left: -55px; + } } .old_line, @@ -490,3 +494,103 @@ } } } + +.diff-comment-avatar-holders { + position: absolute; + height: 19px; + width: 19px; + margin-left: -15px; + + &:hover { + .diff-comment-avatar, + .diff-comments-more-count { + @for $i from 1 through 4 { + $x-pos: 14px; + + &:nth-child(#{$i}) { + @if $i == 4 { + $x-pos: 14.5px; + } + + transform: translateX((($i * $x-pos) - $x-pos)); + + &:hover { + transform: translateX((($i * $x-pos) - $x-pos)) scale(1.2); + } + } + } + } + + .diff-comments-more-count { + padding-left: 2px; + padding-right: 2px; + width: auto; + } + } +} + +.diff-comment-avatar, +.diff-comments-more-count { + position: absolute; + left: 0; + width: 19px; + height: 19px; + margin-right: 0; + border-color: $white-light; + cursor: pointer; + transition: all .1s ease-out; + + @for $i from 1 through 4 { + &:nth-child(#{$i}) { + z-index: (4 - $i); + } + } +} + +.diff-comments-more-count { + width: 19px; + min-width: 19px; + padding-left: 0; + padding-right: 0; + overflow: hidden; +} + +.diff-comments-more-count, +.diff-notes-collapse { + background-color: $gray-darkest; + color: $white-light; + border: 1px solid $white-light; + border-radius: 1em; + font-family: $regular_font; + font-size: 9px; + line-height: 17px; + text-align: center; +} + +.diff-notes-collapse { + position: relative; + width: 19px; + height: 19px; + padding: 0; + transition: transform .1s ease-out; + + svg { + position: absolute; + left: 50%; + top: 50%; + margin-left: -5.5px; + margin-top: -5.5px; + } + + path { + fill: $white-light; + } + + &:hover { + transform: scale(1.2); + } + + &:focus { + outline: 0; + } +} diff --git a/app/views/discussions/_diff_discussion.html.haml b/app/views/discussions/_diff_discussion.html.haml index 2deadbeeceb..ee452add394 100644 --- a/app/views/discussions/_diff_discussion.html.haml +++ b/app/views/discussions/_diff_discussion.html.haml @@ -2,5 +2,5 @@ %tr.notes_holder{ class: ('hide' unless expanded) } %td.notes_line{ colspan: 2 } %td.notes_content - .content + .content{ class: ('hide' unless expanded) } = render "discussions/notes", discussion: discussion diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index cd18ba2ed00..ed279cfe168 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -1,8 +1,11 @@ - email = local_assigns.fetch(:email, false) - plain = local_assigns.fetch(:plain, false) +- discussions = local_assigns.fetch(:discussions, nil) - type = line.type - line_code = diff_file.line_code(line) -%tr.line_holder{ plain ? { class: type} : { class: type, id: line_code } } +- if discussions && !line.meta? + - discussion = discussions[line_code] +%tr.line_holder{ class: type, id: (line_code unless plain) } - case type - when 'match' = diff_match_line line.old_pos, line.new_pos, text: line.text @@ -11,12 +14,14 @@ %td.new_line.diff-line-num %td.line_content.match= line.text - else - %td.old_line.diff-line-num{ class: type, data: { linenumber: line.old_pos } } + %td.old_line.diff-line-num.js-avatar-container{ class: type, data: { linenumber: line.old_pos } } - link_text = type == "new" ? " " : line.old_pos - if plain = link_text - else %a{ href: "##{line_code}", data: { linenumber: link_text } } + - if discussion && !plain + %diff-note-avatars{ "discussion-id" => discussion.id } %td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } } - link_text = type == "old" ? " " : line.new_pos - if plain @@ -29,9 +34,6 @@ - else = diff_line_content(line.text) -- discussions = local_assigns.fetch(:discussions, nil) -- if discussions && !line.meta? - - discussion = discussions[line_code] - - if discussion - - discussion_expanded = local_assigns.fetch(:discussion_expanded, discussion.expanded?) - = render "discussions/diff_discussion", discussion: discussion, expanded: discussion_expanded +- if discussion + - discussion_expanded = local_assigns.fetch(:discussion_expanded, discussion.expanded?) + = render "discussions/diff_discussion", discussion: discussion, expanded: discussion_expanded diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 997bf0fc560..6448748113b 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -4,6 +4,9 @@ - diff_file.parallel_diff_lines.each do |line| - left = line[:left] - right = line[:right] + - last_line = right.new_pos if right + - unless @diff_notes_disabled + - discussion_left, discussion_right = parallel_diff_discussions(left, right, diff_file) %tr.line_holder.parallel - if left - case left.type @@ -15,8 +18,10 @@ - else - left_line_code = diff_file.line_code(left) - left_position = diff_file.position(left) - %td.old_line.diff-line-num{ id: left_line_code, class: left.type, data: { linenumber: left.old_pos } } + %td.old_line.diff-line-num.js-avatar-container{ id: left_line_code, class: left.type, data: { linenumber: left.old_pos } } %a{ href: "##{left_line_code}", data: { linenumber: left.old_pos } } + - if discussion_left + %diff-note-avatars{ "discussion-id" => discussion_left.id } %td.line_content.parallel.noteable_line{ class: left.type, data: diff_view_line_data(left_line_code, left_position, 'old') }= diff_line_content(left.text) - else %td.old_line.diff-line-num.empty-cell @@ -32,17 +37,17 @@ - else - right_line_code = diff_file.line_code(right) - right_position = diff_file.position(right) - %td.new_line.diff-line-num{ id: right_line_code, class: right.type, data: { linenumber: right.new_pos } } + %td.new_line.diff-line-num.js-avatar-container{ id: right_line_code, class: right.type, data: { linenumber: right.new_pos } } %a{ href: "##{right_line_code}", data: { linenumber: right.new_pos } } + - if discussion_right + %diff-note-avatars{ "discussion-id" => discussion_right.id } %td.line_content.parallel.noteable_line{ class: right.type, data: diff_view_line_data(right_line_code, right_position, 'new') }= diff_line_content(right.text) - else %td.old_line.diff-line-num.empty-cell %td.line_content.parallel - - unless @diff_notes_disabled - - discussion_left, discussion_right = parallel_diff_discussions(left, right, diff_file) - - if discussion_left || discussion_right - = render "discussions/parallel_diff_discussion", discussion_left: discussion_left, discussion_right: discussion_right + - if discussion_left || discussion_right + = render "discussions/parallel_diff_discussion", discussion_left: discussion_left, discussion_right: discussion_right - if !diff_file.new_file && !diff_file.deleted_file && diff_file.diff_lines.any? - last_line = diff_file.diff_lines.last - if last_line.new_pos < total_lines diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index a73e8f345e0..a7618370a5d 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -2,7 +2,7 @@ - return if note.cross_reference_not_visible_for?(current_user) - note_editable = note_editable?(note) -%li.timeline-entry{ id: dom_id(note), class: ["note", "note-row-#{note.id}", ('system-note' if note.system)], data: {author_id: note.author.id, editable: note_editable} } +%li.timeline-entry{ id: dom_id(note), class: ["note", "note-row-#{note.id}", ('system-note' if note.system)], data: {author_id: note.author.id, editable: note_editable, note_id: note.id} } .timeline-entry-inner .timeline-icon %a{ href: user_path(note.author) } @@ -30,11 +30,15 @@ - if note.resolvable? - can_resolve = can?(current_user, :resolve_note, note) - %resolve-btn{ "discussion-id" => "#{note.discussion_id}", + %resolve-btn{ "project-path" => project_path(note.project), + "discussion-id" => note.discussion_id, ":note-id" => note.id, ":resolved" => note.resolved?, ":can-resolve" => can_resolve, - "resolved-by" => "#{note.resolved_by.try(:name)}", + ":author-name" => "'#{j(note.author.name)}'", + "author-avatar" => note.author.avatar_url, + ":note-truncated" => "'#{truncate(note.note, length: 17)}'", + ":resolved-by" => "'#{j(note.resolved_by.try(:name))}'", "v-show" => "#{can_resolve || note.resolved?}", "inline-template" => true, "ref" => "note_#{note.id}" } diff --git a/app/views/shared/icons/_collapse.svg.erb b/app/views/shared/icons/_collapse.svg.erb new file mode 100644 index 00000000000..917753fb343 --- /dev/null +++ b/app/views/shared/icons/_collapse.svg.erb @@ -0,0 +1 @@ +<svg width="<%= size %>" height="<%= size %>" viewBox="0 0 9 13"><path d="M2.57568253,6.49866948 C2.50548852,6.57199715 2.44637866,6.59708255 2.39835118,6.57392645 C2.3503237,6.55077034 2.32631032,6.48902165 2.32631032,6.38867852 L2.32631032,-2.13272614 C2.32631032,-2.23306927 2.3503237,-2.29481796 2.39835118,-2.31797406 C2.44637866,-2.34113017 2.50548852,-2.31604477 2.57568253,-2.24271709 L6.51022184,1.86747129 C6.53977721,1.8983461 6.56379059,1.93500939 6.5822627,1.97746225 L6.5822627,2.27849013 C6.56379059,2.31708364 6.53977721,2.35374693 6.51022184,2.38848109 L2.57568253,6.49866948 Z" transform="translate(4.454287, 2.127976) rotate(90.000000) translate(-4.454287, -2.127976) "></path><path d="M3.74312342,2.09553332 C3.74312342,1.99519019 3.77821989,1.9083561 3.8484139,1.83502843 C3.91860791,1.76170075 4.00173115,1.72503747 4.09778611,1.72503747 L4.80711151,1.72503747 C4.90316647,1.72503747 4.98628971,1.76170075 5.05648372,1.83502843 C5.12667773,1.9083561 5.16177421,1.99519019 5.16177421,2.09553332 L5.16177421,10.2464421 C5.16177421,10.3467853 5.12667773,10.4336194 5.05648372,10.506947 C4.98628971,10.5802747 4.90316647,10.616938 4.80711151,10.616938 L4.09778611,10.616938 C4.00173115,10.616938 3.91860791,10.5802747 3.8484139,10.506947 C3.77821989,10.4336194 3.74312342,10.3467853 3.74312342,10.2464421 L3.74312342,2.09553332 Z" transform="translate(4.452449, 6.170988) rotate(-90.000000) translate(-4.452449, -6.170988) "></path><path d="M2.57568253,14.6236695 C2.50548852,14.6969971 2.44637866,14.7220826 2.39835118,14.6989264 C2.3503237,14.6757703 2.32631032,14.6140216 2.32631032,14.5136785 L2.32631032,5.99227386 C2.32631032,5.89193073 2.3503237,5.83018204 2.39835118,5.80702594 C2.44637866,5.78386983 2.50548852,5.80895523 2.57568253,5.88228291 L6.51022184,9.99247129 C6.53977721,10.0233461 6.56379059,10.0600094 6.5822627,10.1024622 L6.5822627,10.4034901 C6.56379059,10.4420836 6.53977721,10.4787469 6.51022184,10.5134811 L2.57568253,14.6236695 Z" transform="translate(4.454287, 10.252976) scale(1, -1) rotate(90.000000) translate(-4.454287, -10.252976) "></path></svg> diff --git a/spec/features/merge_requests/diff_notes_avatars_spec.rb b/spec/features/merge_requests/diff_notes_avatars_spec.rb new file mode 100644 index 00000000000..7df102067d6 --- /dev/null +++ b/spec/features/merge_requests/diff_notes_avatars_spec.rb @@ -0,0 +1,136 @@ +require 'spec_helper' + +feature 'Diff note avatars', feature: true, js: true do + include WaitForAjax + + let(:user) { create(:user) } + let(:project) { create(:project, :public) } + let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: "Bug NS-04") } + let(:path) { "files/ruby/popen.rb" } + let(:position) do + Gitlab::Diff::Position.new( + old_path: path, + new_path: path, + old_line: nil, + new_line: 9, + diff_refs: merge_request.diff_refs + ) + end + let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) } + + before do + project.team << [user, :master] + login_as user + end + + %w(inline parallel).each do |view| + context "#{view} view" do + before do + visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, view: view) + + wait_for_ajax + end + + it 'shows note avatar' do + page.within find("[id='#{position.line_code(project.repository)}']") do + find('.diff-notes-collapse').click + + expect(page).to have_selector('img.js-diff-comment-avatar', count: 1) + end + end + + it 'shows comment on note avatar' do + page.within find("[id='#{position.line_code(project.repository)}']") do + find('.diff-notes-collapse').click + + expect(first('img.js-diff-comment-avatar')["title"]).to eq("#{note.author.name}: #{note.note.truncate(17)}") + end + end + + it 'toggles comments when clicking avatar' do + page.within find("[id='#{position.line_code(project.repository)}']") do + find('.diff-notes-collapse').click + end + + expect(page).to have_selector('.notes_holder', visible: false) + + page.within find("[id='#{position.line_code(project.repository)}']") do + first('img.js-diff-comment-avatar').click + end + + expect(page).to have_selector('.notes_holder') + end + + it 'removes avatar when note is deleted' do + page.within find(".note-row-#{note.id}") do + find('.js-note-delete').click + end + + wait_for_ajax + + page.within find("[id='#{position.line_code(project.repository)}']") do + expect(page).not_to have_selector('img.js-diff-comment-avatar') + end + end + + it 'adds avatar when commenting' do + click_button 'Reply...' + + page.within '.js-discussion-note-form' do + find('.js-note-text').native.send_keys('Test') + + click_button 'Comment' + + wait_for_ajax + end + + page.within find("[id='#{position.line_code(project.repository)}']") do + find('.diff-notes-collapse').click + + expect(page).to have_selector('img.js-diff-comment-avatar', count: 2) + end + end + + it 'adds multiple comments' do + 3.times do + click_button 'Reply...' + + page.within '.js-discussion-note-form' do + find('.js-note-text').native.send_keys('Test') + + find('.js-comment-button').trigger 'click' + + wait_for_ajax + end + end + + page.within find("[id='#{position.line_code(project.repository)}']") do + find('.diff-notes-collapse').click + + expect(page).to have_selector('img.js-diff-comment-avatar', count: 3) + expect(find('.diff-comments-more-count')).to have_content '+1' + end + end + + context 'multiple comments' do + before do + create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) + create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) + create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) + + visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, view: view) + + wait_for_ajax + end + + it 'shows extra comment count' do + page.within find("[id='#{position.line_code(project.repository)}']") do + find('.diff-notes-collapse').click + + expect(find('.diff-comments-more-count')).to have_content '+1' + end + end + end + end + end +end diff --git a/spec/javascripts/diff_comments_store_spec.js b/spec/javascripts/diff_comments_store_spec.js index f956394ef53..84cf98c930a 100644 --- a/spec/javascripts/diff_comments_store_spec.js +++ b/spec/javascripts/diff_comments_store_spec.js @@ -7,7 +7,16 @@ require('~/diff_notes/stores/comments'); (() => { function createDiscussion(noteId = 1, resolved = true) { - CommentsStore.create('a', noteId, true, resolved, 'test'); + CommentsStore.create({ + discussionId: 'a', + noteId, + canResolve: true, + resolved, + resolvedBy: 'test', + authorName: 'test', + authorAvatar: 'test', + noteTruncated: 'test...', + }); } beforeEach(() => { |