Merge branch '32125-fix-link-to-note-in-diff' into 'master'

Fix linking to resolved note in diff

Closes #32125

See merge request !11327

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();

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');
+          }
         },
       });
     }

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);
@@ -102,9 +102,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
@@ -796,10 +796,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')
@@ -869,35 +873,43 @@ const normalizeNewlines = function(str) {
     Sets up the form and shows it.
      */
 
-    Notes.prototype.addDiffNote = function(e) {
+    Notes.prototype.onAddDiffNote = function(e) {
-      var $link, addForm, hasNotes, lineType, newForm, nextRow, noteForm, notesContent, notesContentSelector, replyButton, row, rowCssToAdd, targetContent, isDiffCommentAvatar;
       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();
+      const nextRow = row.next();
-      hasNotes = nextRow.is(".notes_holder");
+      let targetRow = row;
+      if (nextRow.is('.notes_holder')) {
+        targetRow = nextRow;
+      }
+
+      hasNotes = targetRow.is(".notes_holder");
       addForm = false;
-      notesContentSelector = ".notes_content";
+      let lineTypeSelector = '';
       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");
+        lineTypeSelector = `.${lineType}`;
-        notesContentSelector += "." + lineType;
         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";
+      const notesContentSelector = `.notes_content${lineTypeSelector} .content`;
-      notesContent = nextRow.find(notesContentSelector);
+      let notesContent = targetRow.find(notesContentSelector);
 
-      if (hasNotes && !isDiffCommentAvatar) {
+      if (hasNotes && showReplyInput) {
-        nextRow.show();
+        targetRow.show();
-        notesContent = nextRow.find(notesContentSelector);
+        notesContent = targetRow.find(notesContentSelector);
         if (notesContent.length) {
           notesContent.show();
           replyButton = notesContent.find(".js-discussion-reply-button:visible");
           if (replyButton.length) {
-            e.target = replyButton[0];
+            this.replyToDiscussionNote(replyButton[0]);
-            $.proxy(this.replyToDiscussionNote, replyButton[0], e).call();
           } else {
             // In parallel view, the form may not be present in one of the panes
             noteForm = notesContent.find(".js-discussion-note-form");
@@ -906,18 +918,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();
+        targetRow = row.next();
-        notesContent = nextRow.find(notesContentSelector);
+        notesContent = targetRow.find(notesContentSelector);
         addForm = true;
       } else {
-        nextRow.show();
+        targetRow.show();
         notesContent.toggle(!notesContent.is(':visible'));
 
-        if (!nextRow.find('.content:not(:empty)').is(':visible')) {
+        if (!targetRow.find('.content:not(:empty)').is(':visible')) {
-          nextRow.hide();
+          targetRow.hide();
         }
       }
 
@@ -1331,7 +1343,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');
           }
 

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-entry .timeline-entry-inner {
-      .timeline-icon {
+    .timeline-icon {
-        display: none;
+      display: none;
-      }
+    }
 
-      .timeline-content {
+    .timeline-content {
-        margin-left: 0;
+      margin-left: 0;
-      }
     }
   }
 }