From 16da7f2365fe67c078a2f10766662b93fe8576fc Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 12 May 2017 05:19:32 -0500 Subject: [PATCH] Fix linking to resolved note in diff Fix https://gitlab.com/gitlab-org/gitlab-ce/issues/32125 --- .../components/diff_note_avatars.js | 2 +- app/assets/javascripts/merge_request_tabs.js | 18 ++++- app/assets/javascripts/notes.js | 70 +++++++++++-------- .../stylesheets/framework/timeline.scss | 63 +++++++++-------- 4 files changed, 91 insertions(+), 62 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 index f3a688fbf2f..5f533b5761c 100644 --- a/app/assets/javascripts/diff_notes/components/diff_note_avatars.js +++ b/app/assets/javascripts/diff_notes/components/diff_note_avatars.js @@ -120,7 +120,7 @@ const DiffNoteAvatars = Vue.extend({ }, methods: { clickedAvatar(e) { - notes.addDiffNote(e); + notes.onAddDiffNote(e); // Toggle the active state of the toggle all button this.toggleDiscussionsToggleState(); diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 93c30c54a8e..efbfdbcee0c 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -1,6 +1,7 @@ /* eslint-disable no-new, class-methods-use-this */ /* global Breakpoints */ /* global Flash */ +/* global notes */ import Cookies from 'js-cookie'; import './breakpoints'; @@ -251,7 +252,8 @@ import BlobForkSuggestion from './blob/blob_fork_suggestion'; this.ajaxGet({ url: `${urlPathname}.json${location.search}`, success: (data) => { - $('#diffs').html(data.html); + const $container = $('#diffs'); + $container.html(data.html); if (typeof gl.diffNotesCompileComponents !== 'undefined') { gl.diffNotesCompileComponents(); @@ -278,6 +280,20 @@ import BlobForkSuggestion from './blob/blob_fork_suggestion'; }) .init(); }); + + // Scroll any linked note into view + // Similar to `toggler_behavior` in the discussion tab + const hash = window.gl.utils.getLocationHash(); + const anchor = hash && $container.find(`[id="${hash}"]`); + if (anchor) { + const notesContent = anchor.closest('.notes_content'); + const lineType = notesContent.hasClass('new') ? 'new' : 'old'; + notes.addDiffNote(anchor, lineType, false); + anchor[0].scrollIntoView(); + // We have multiple elements on the page with `#note_xxx` + // (discussion and diff tabs) and `:target` only applies to the first + anchor.addClass('target'); + } }, }); } diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index f6fe6d9f0fd..73396ca47fe 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -33,9 +33,9 @@ const normalizeNewlines = function(str) { this.updateComment = this.updateComment.bind(this); this.visibilityChange = this.visibilityChange.bind(this); this.cancelDiscussionForm = this.cancelDiscussionForm.bind(this); - this.addDiffNote = this.addDiffNote.bind(this); + this.onAddDiffNote = this.onAddDiffNote.bind(this); this.setupDiscussionNoteForm = this.setupDiscussionNoteForm.bind(this); - this.replyToDiscussionNote = this.replyToDiscussionNote.bind(this); + this.onReplyToDiscussionNote = this.onReplyToDiscussionNote.bind(this); this.removeNote = this.removeNote.bind(this); this.cancelEdit = this.cancelEdit.bind(this); this.updateNote = this.updateNote.bind(this); @@ -100,9 +100,9 @@ const normalizeNewlines = function(str) { // update the file name when an attachment is selected $(document).on("change", ".js-note-attachment-input", this.updateFormAttachment); // reply to diff/discussion notes - $(document).on("click", ".js-discussion-reply-button", this.replyToDiscussionNote); + $(document).on("click", ".js-discussion-reply-button", this.onReplyToDiscussionNote); // add diff note - $(document).on("click", ".js-add-diff-note-button", this.addDiffNote); + $(document).on("click", ".js-add-diff-note-button", this.onAddDiffNote); // hide diff note form $(document).on("click", ".js-close-discussion-note-form", this.cancelDiscussionForm); // toggle commit list @@ -794,10 +794,14 @@ const normalizeNewlines = function(str) { Shows the note form below the notes. */ - Notes.prototype.replyToDiscussionNote = function(e) { + Notes.prototype.onReplyToDiscussionNote = function(e) { + this.replyToDiscussionNote(e.target); + }; + + Notes.prototype.replyToDiscussionNote = function(target) { var form, replyLink; form = this.cleanForm(this.formClone.clone()); - replyLink = $(e.target).closest(".js-discussion-reply-button"); + replyLink = $(target).closest(".js-discussion-reply-button"); // insert the form after the button replyLink .closest('.discussion-reply-holder') @@ -867,35 +871,43 @@ const normalizeNewlines = function(str) { Sets up the form and shows it. */ - Notes.prototype.addDiffNote = function(e) { - var $link, addForm, hasNotes, lineType, newForm, nextRow, noteForm, notesContent, notesContentSelector, replyButton, row, rowCssToAdd, targetContent, isDiffCommentAvatar; + Notes.prototype.onAddDiffNote = function(e) { e.preventDefault(); - $link = $(e.currentTarget || e.target); + const $link = $(e.currentTarget || e.target); + const showReplyInput = !$link.hasClass('js-diff-comment-avatar'); + this.addDiffNote($link, $link.data('lineType'), showReplyInput); + }; + + Notes.prototype.addDiffNote = function(target, lineType, showReplyInput) { + var $link, addForm, hasNotes, newForm, noteForm, replyButton, row, rowCssToAdd, targetContent, isDiffCommentAvatar; + $link = $(target); row = $link.closest("tr"); - nextRow = row.next(); - hasNotes = nextRow.is(".notes_holder"); + const nextRow = row.next(); + let targetRow = row; + if (nextRow.is('.notes_holder')) { + targetRow = nextRow; + } + + hasNotes = targetRow.is(".notes_holder"); addForm = false; - notesContentSelector = ".notes_content"; + let lineTypeSelector = ''; rowCssToAdd = "
"; - isDiffCommentAvatar = $link.hasClass('js-diff-comment-avatar'); // In parallel view, look inside the correct left/right pane if (this.isParallelView()) { - lineType = $link.data("lineType"); - notesContentSelector += "." + lineType; + lineTypeSelector = `.${lineType}`; rowCssToAdd = "
"; } - notesContentSelector += " .content"; - notesContent = nextRow.find(notesContentSelector); + const notesContentSelector = `.notes_content${lineTypeSelector} .content`; + let notesContent = targetRow.find(notesContentSelector); - if (hasNotes && !isDiffCommentAvatar) { - nextRow.show(); - notesContent = nextRow.find(notesContentSelector); + if (hasNotes && showReplyInput) { + targetRow.show(); + notesContent = targetRow.find(notesContentSelector); if (notesContent.length) { notesContent.show(); replyButton = notesContent.find(".js-discussion-reply-button:visible"); if (replyButton.length) { - e.target = replyButton[0]; - $.proxy(this.replyToDiscussionNote, replyButton[0], e).call(); + this.replyToDiscussionNote(replyButton[0]); } else { // In parallel view, the form may not be present in one of the panes noteForm = notesContent.find(".js-discussion-note-form"); @@ -904,18 +916,18 @@ const normalizeNewlines = function(str) { } } } - } else if (!isDiffCommentAvatar) { + } else if (showReplyInput) { // add a notes row and insert the form row.after(rowCssToAdd); - nextRow = row.next(); - notesContent = nextRow.find(notesContentSelector); + targetRow = row.next(); + notesContent = targetRow.find(notesContentSelector); addForm = true; } else { - nextRow.show(); + targetRow.show(); notesContent.toggle(!notesContent.is(':visible')); - if (!nextRow.find('.content:not(:empty)').is(':visible')) { - nextRow.hide(); + if (!targetRow.find('.content:not(:empty)').is(':visible')) { + targetRow.hide(); } } @@ -1320,7 +1332,7 @@ const normalizeNewlines = function(str) { // Show form again on UI on failure if (isDiscussionForm && $notesContainer.length) { const replyButton = $notesContainer.parent().find('.js-discussion-reply-button'); - $.proxy(this.replyToDiscussionNote, replyButton[0], { target: replyButton[0] }).call(); + this.replyToDiscussionNote(replyButton[0]); $form = $notesContainer.parent().find('form'); } diff --git a/app/assets/stylesheets/framework/timeline.scss b/app/assets/stylesheets/framework/timeline.scss index d2164a1d333..aa0c512a277 100644 --- a/app/assets/stylesheets/framework/timeline.scss +++ b/app/assets/stylesheets/framework/timeline.scss @@ -3,30 +3,6 @@ margin: 0; padding: 0; - .timeline-entry { - padding: $gl-padding $gl-btn-padding 0; - border-color: $white-normal; - color: $gl-text-color; - border-bottom: 1px solid $border-white-light; - - .timeline-entry-inner { - position: relative; - } - - &:target { - background: $line-target-blue; - } - - .avatar { - margin-right: 15px; - } - - .controls { - padding-top: 10px; - float: right; - } - } - .note-text { p:last-child { margin-bottom: 0; @@ -46,20 +22,45 @@ } } +.timeline-entry { + padding: $gl-padding $gl-btn-padding 0; + border-color: $white-normal; + color: $gl-text-color; + border-bottom: 1px solid $border-white-light; + + .timeline-entry-inner { + position: relative; + } + + &:target, + &.target { + background: $line-target-blue; + } + + .avatar { + margin-right: 15px; + } + + .controls { + padding-top: 10px; + float: right; + } +} + @media (max-width: $screen-xs-max) { .timeline { &::before { background: none; } + } - .timeline-entry .timeline-entry-inner { - .timeline-icon { - display: none; - } + .timeline-entry .timeline-entry-inner { + .timeline-icon { + display: none; + } - .timeline-content { - margin-left: 0; - } + .timeline-content { + margin-left: 0; } } }