Merge branch 'inject-add-diff-note' into 'master'

Render add-diff-note with server and optimize styles

See merge request !12103

app/assets/javascripts/diff.js

 /* eslint-disable class-methods-use-this */
 
 import './lib/utils/url_utility';
+import FilesCommentButton from './files_comment_button';
 
 const UNFOLD_COUNT = 20;
 let isBound = false;
@@ -8,8 +9,10 @@ let isBound = false;
 class Diff {
   constructor() {
     const $diffFile = $('.files .diff-file');
+
     $diffFile.singleFileDiff();
-    $diffFile.filesCommentButton();
+
+    FilesCommentButton.init($diffFile);
 
     $diffFile.each((index, file) => new gl.ImageFile(file));
 

app/assets/javascripts/diff_notes/components/diff_note_avatars.js

@@ -139,9 +139,9 @@ const DiffNoteAvatars = Vue.extend({
       const notesCount = this.notesCount;
 
       $(this.$el).closest('.js-avatar-container')
-        .toggleClass('js-no-comment-btn', notesCount > 0)
+        .toggleClass('no-comment-btn', notesCount > 0)
         .nextUntil('.js-avatar-container')
-        .toggleClass('js-no-comment-btn', notesCount > 0);
+        .toggleClass('no-comment-btn', notesCount > 0);
     },
     toggleDiscussionsToggleState() {
       const $notesHolders = $(this.$el).closest('.code').find('.notes_holder');

app/assets/javascripts/files_comment_button.js

 /* eslint-disable func-names, space-before-function-paren, no-var, prefer-rest-params, wrap-iife, max-len, one-var, one-var-declaration-per-line, quotes, prefer-template, newline-per-chained-call, comma-dangle, new-cap, no-else-return, consistent-return */
-/* global FilesCommentButton */
 /* global notes */
 
-let $commentButtonTemplate;
-
-window.FilesCommentButton = (function() {
-  var COMMENT_BUTTON_CLASS, EMPTY_CELL_CLASS, LINE_COLUMN_CLASSES, LINE_CONTENT_CLASS, LINE_HOLDER_CLASS, LINE_NUMBER_CLASS, OLD_LINE_CLASS, TEXT_FILE_SELECTOR, UNFOLDABLE_LINE_CLASS;
-
-  COMMENT_BUTTON_CLASS = '.add-diff-note';
-
-  LINE_HOLDER_CLASS = '.line_holder';
-
-  LINE_NUMBER_CLASS = 'diff-line-num';
-
-  LINE_CONTENT_CLASS = 'line_content';
-
-  UNFOLDABLE_LINE_CLASS = 'js-unfold';
-
-  EMPTY_CELL_CLASS = 'empty-cell';
-
-  OLD_LINE_CLASS = 'old_line';
-
-  LINE_COLUMN_CLASSES = "." + LINE_NUMBER_CLASS + ", .line_content";
-
-  TEXT_FILE_SELECTOR = '.text-file';
-
-  function FilesCommentButton(filesContainerElement) {
-    this.render = this.render.bind(this);
-    this.hideButton = this.hideButton.bind(this);
-    this.isParallelView = notes.isParallelView();
-    filesContainerElement.on('mouseover', LINE_COLUMN_CLASSES, this.render)
-      .on('mouseleave', LINE_COLUMN_CLASSES, this.hideButton);
+/* Developer beware! Do not add logic to showButton or hideButton
+ * that will force a reflow. Doing so will create a signficant performance
+ * bottleneck for pages with large diffs. For a comprehensive list of what
+ * causes reflows, visit https://gist.github.com/paulirish/5d52fb081b3570c81e3a
+ */
+
+const LINE_NUMBER_CLASS = 'diff-line-num';
+const UNFOLDABLE_LINE_CLASS = 'js-unfold';
+const NO_COMMENT_CLASS = 'no-comment-btn';
+const EMPTY_CELL_CLASS = 'empty-cell';
+const OLD_LINE_CLASS = 'old_line';
+const LINE_COLUMN_CLASSES = `.${LINE_NUMBER_CLASS}, .line_content`;
+const DIFF_CONTAINER_SELECTOR = '.files';
+const DIFF_EXPANDED_CLASS = 'diff-expanded';
+
+export default {
+  init($diffFile) {
+    /* Caching is used only when the following members are *true*. This is because there are likely to be
+     * differently configured versions of diffs in the same session. However if these values are true, they
+     * will be true in all cases */
+
+    if (!this.userCanCreateNote) {
+      // data-can-create-note is an empty string when true, otherwise undefined
+      this.userCanCreateNote = $diffFile.closest(DIFF_CONTAINER_SELECTOR).data('can-create-note') === '';
     }
 
-  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);
-
-    if (!this.validateButtonParent(buttonParentElement) || !this.validateLineContent(lineContentElement)) return;
-
-    $button = $(COMMENT_BUTTON_CLASS, buttonParentElement);
-    buttonParentElement.addClass('is-over')
-      .nextUntil(`.${LINE_CONTENT_CLASS}`).addClass('is-over');
-
-    if ($button.length) {
-      return;
+    if (typeof notes !== 'undefined' && !this.isParallelView) {
+      this.isParallelView = notes.isParallelView && notes.isParallelView();
     }
 
-    textFileElement = this.getTextFileElement($currentTarget);
-    buttonParentElement.append(this.buildButton({
-      discussionID: lineContentElement.attr('data-discussion-id'),
-      lineType: lineContentElement.attr('data-line-type'),
-
-      noteableType: textFileElement.attr('data-noteable-type'),
-      noteableID: textFileElement.attr('data-noteable-id'),
-      commitID: textFileElement.attr('data-commit-id'),
-      noteType: lineContentElement.attr('data-note-type'),
-
-      // LegacyDiffNote
-      lineCode: lineContentElement.attr('data-line-code'),
-
-      // DiffNote
-      position: lineContentElement.attr('data-position')
-    }));
-  };
-
-  FilesCommentButton.prototype.hideButton = function(e) {
-    var $currentTarget = $(e.currentTarget);
-    var buttonParentElement = this.getButtonParent($currentTarget);
-
-    buttonParentElement.removeClass('is-over')
-      .nextUntil(`.${LINE_CONTENT_CLASS}`).removeClass('is-over');
-  };
-
-  FilesCommentButton.prototype.buildButton = function(buttonAttributes) {
-    return $commentButtonTemplate.clone().attr({
-      'data-discussion-id': buttonAttributes.discussionID,
-      'data-line-type': buttonAttributes.lineType,
-
-      'data-noteable-type': buttonAttributes.noteableType,
-      'data-noteable-id': buttonAttributes.noteableID,
-      'data-commit-id': buttonAttributes.commitID,
-      'data-note-type': buttonAttributes.noteType,
-
-      // LegacyDiffNote
-      'data-line-code': buttonAttributes.lineCode,
-
-      // DiffNote
-      'data-position': buttonAttributes.position
-    });
-  };
-
-  FilesCommentButton.prototype.getTextFileElement = function(hoveredElement) {
-    return hoveredElement.closest(TEXT_FILE_SELECTOR);
-  };
-
-  FilesCommentButton.prototype.getLineContent = function(hoveredElement) {
-    if (hoveredElement.hasClass(LINE_CONTENT_CLASS)) {
-      return hoveredElement;
-    }
-    if (!this.isParallelView) {
-      return $(hoveredElement).closest(LINE_HOLDER_CLASS).find("." + LINE_CONTENT_CLASS);
-    } else {
-      return $(hoveredElement).next("." + LINE_CONTENT_CLASS);
+    if (this.userCanCreateNote) {
+      $diffFile.on('mouseover', LINE_COLUMN_CLASSES, e => this.showButton(this.isParallelView, e))
+        .on('mouseleave', LINE_COLUMN_CLASSES, e => this.hideButton(this.isParallelView, e));
     }
-  };
+  },
 
-  FilesCommentButton.prototype.getButtonParent = function(hoveredElement) {
-    if (!this.isParallelView) {
-      if (hoveredElement.hasClass(OLD_LINE_CLASS)) {
-        return hoveredElement;
-      }
-      return hoveredElement.parent().find("." + OLD_LINE_CLASS);
-    } else {
-      if (hoveredElement.hasClass(LINE_NUMBER_CLASS)) {
-        return hoveredElement;
-      }
-      return $(hoveredElement).prev("." + LINE_NUMBER_CLASS);
-    }
-  };
+  showButton(isParallelView, e) {
+    const buttonParentElement = this.getButtonParent(e.currentTarget, isParallelView);
 
-  FilesCommentButton.prototype.validateButtonParent = function(buttonParentElement) {
-    return !buttonParentElement.hasClass(EMPTY_CELL_CLASS) && !buttonParentElement.hasClass(UNFOLDABLE_LINE_CLASS);
-  };
+    if (!this.validateButtonParent(buttonParentElement)) return;
 
-  FilesCommentButton.prototype.validateLineContent = function(lineContentElement) {
-    return lineContentElement.attr('data-note-type') && lineContentElement.attr('data-note-type') !== '';
-  };
+    buttonParentElement.classList.add('is-over');
+    buttonParentElement.nextElementSibling.classList.add('is-over');
+  },
 
-  return FilesCommentButton;
-})();
+  hideButton(isParallelView, e) {
+    const buttonParentElement = this.getButtonParent(e.currentTarget, isParallelView);
 
-$.fn.filesCommentButton = function() {
-  $commentButtonTemplate = $('<button name="button" type="submit" class="add-diff-note js-add-diff-note-button" title="Add a comment to this line"><i class="fa fa-comment-o"></i></button>');
+    buttonParentElement.classList.remove('is-over');
+    buttonParentElement.nextElementSibling.classList.remove('is-over');
+  },
 
-  if (!(this && (this.parent().data('can-create-note') != null))) {
-    return;
+  getButtonParent(hoveredElement, isParallelView) {
+    if (isParallelView) {
+      if (!hoveredElement.classList.contains(LINE_NUMBER_CLASS)) {
+        return hoveredElement.previousElementSibling;
       }
-  return this.each(function() {
-    if (!$.data(this, 'filesCommentButton')) {
-      return $.data(this, 'filesCommentButton', new FilesCommentButton($(this)));
+    } else if (!hoveredElement.classList.contains(OLD_LINE_CLASS)) {
+      return hoveredElement.parentNode.querySelector(`.${OLD_LINE_CLASS}`);
     }
-  });
+    return hoveredElement;
+  },
+
+  validateButtonParent(buttonParentElement) {
+    return !buttonParentElement.classList.contains(EMPTY_CELL_CLASS) &&
+      !buttonParentElement.classList.contains(UNFOLDABLE_LINE_CLASS) &&
+      !buttonParentElement.classList.contains(NO_COMMENT_CLASS) &&
+      !buttonParentElement.parentNode.classList.contains(DIFF_EXPANDED_CLASS);
+  },
 };

app/assets/javascripts/notes.js

@@ -829,6 +829,8 @@ export default class Notes {
    */
   setupDiscussionNoteForm(dataHolder, form) {
     // setup note target
+    const diffFileData = dataHolder.closest('.text-file');
+
     var discussionID = dataHolder.data('discussionId');
 
     if (discussionID) {
@@ -839,9 +841,10 @@ export default class Notes {
     form.attr('data-line-code', dataHolder.data('lineCode'));
     form.find('#line_type').val(dataHolder.data('lineType'));
 
-    form.find('#note_noteable_type').val(dataHolder.data('noteableType'));
-    form.find('#note_noteable_id').val(dataHolder.data('noteableId'));
-    form.find('#note_commit_id').val(dataHolder.data('commitId'));
+    form.find('#note_noteable_type').val(diffFileData.data('noteableType'));
+    form.find('#note_noteable_id').val(diffFileData.data('noteableId'));
+    form.find('#note_commit_id').val(diffFileData.data('commitId'));
+
     form.find('#note_type').val(dataHolder.data('noteType'));
 
     // LegacyDiffNote

app/assets/javascripts/single_file_diff.js

 /* eslint-disable func-names, prefer-arrow-callback, space-before-function-paren, no-var, prefer-rest-params, wrap-iife, one-var, one-var-declaration-per-line, consistent-return, no-param-reassign, max-len */
 
+import FilesCommentButton from './files_comment_button';
+
 (function() {
   window.SingleFileDiff = (function() {
     var COLLAPSED_HTML, ERROR_HTML, LOADING_HTML, WRAPPER;
@@ -78,6 +80,8 @@
             gl.diffNotesCompileComponents();
           }
 
+          FilesCommentButton.init($(_this.file));
+
           if (cb) cb();
         };
       })(this));

app/assets/stylesheets/pages/diff.scss

@@ -476,6 +476,7 @@
   height: 19px;
   width: 19px;
   margin-left: -15px;
+  z-index: 100;
 
   &:hover {
     .diff-comment-avatar,
@@ -491,7 +492,7 @@
           transform: translateX((($i * $x-pos) - $x-pos));
 
           &:hover {
-            transform: translateX((($i * $x-pos) - $x-pos)) scale(1.2);
+            transform: translateX((($i * $x-pos) - $x-pos));
           }
         }
       }
@@ -542,6 +543,7 @@
   height: 19px;
   padding: 0;
   transition: transform .1s ease-out;
+  z-index: 100;
 
   svg {
     position: absolute;
@@ -555,10 +557,6 @@
     fill: $white-light;
   }
 
-  &:hover {
-    transform: scale(1.2);
-  }
-
   &:focus {
     outline: 0;
   }

app/assets/stylesheets/pages/notes.scss

@@ -628,8 +628,14 @@ ul.notes {
  * Line note button on the side of diffs
  */
 
+.line_holder .is-over:not(.no-comment-btn) {
+  .add-diff-note {
+    opacity: 1;
+  }
+}
+
 .add-diff-note {
-  display: none;
+  opacity: 0;
   margin-top: -2px;
   border-radius: 50%;
   background: $white-light;
@@ -642,13 +648,11 @@ ul.notes {
   width: 23px;
   height: 23px;
   border: 1px solid $blue-500;
-  transition: transform .1s ease-in-out;
 
   &:hover {
     background: $blue-500;
     border-color: $blue-600;
     color: $white-light;
-    transform: scale(1.15);
   }
 
   &:active {

app/helpers/notes_helper.rb

@@ -47,6 +47,18 @@ module NotesHelper
     data
   end
 
+  def add_diff_note_button(line_code, position, line_type)
+    return if @diff_notes_disabled
+
+    button_tag '',
+      class: 'add-diff-note js-add-diff-note-button',
+      type: 'submit', name: 'button',
+      data: diff_view_line_data(line_code, position, line_type),
+      title: 'Add a comment to this line' do
+      icon('comment-o')
+    end
+  end
+
   def link_to_reply_discussion(discussion, line_type = nil)
     return unless current_user
 

app/views/projects/diffs/_line.html.haml

@@ -19,6 +19,7 @@
       - if plain
         = link_text
       - else
+        = add_diff_note_button(line_code, diff_file.position(line), type)
         %a{ href: "##{line_code}", data: { linenumber: link_text } }
       - discussion = line_discussions.try(:first)
       - if discussion && discussion.resolvable? && !plain
@@ -29,7 +30,7 @@
         = link_text
       - else
         %a{ href: "##{line_code}", data: { linenumber: link_text } }
-    %td.line_content.noteable_line{ class: type, data: (diff_view_line_data(line_code, diff_file.position(line), type) unless plain) }<
+    %td.line_content.noteable_line{ class: type }<
       - if email
         %pre= line.text
       - else

app/views/projects/diffs/_parallel_view.html.haml

 / Side-by-side diff view
+
 .text-file.diff-wrap-lines.code.js-syntax-highlight{ data: diff_view_data }
   %table
     - diff_file.parallel_diff_lines.each do |line|
@@ -18,11 +19,12 @@
             - left_line_code = diff_file.line_code(left)
             - left_position = diff_file.position(left)
             %td.old_line.diff-line-num.js-avatar-container{ id: left_line_code, class: left.type, data: { linenumber: left.old_pos } }
+              = add_diff_note_button(left_line_code, left_position, 'old')
               %a{ href: "##{left_line_code}", data: { linenumber: left.old_pos } }
               - discussion_left = discussions_left.try(:first)
               - if discussion_left && discussion_left.resolvable?
                 %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)
+            %td.line_content.parallel.noteable_line{ class: left.type }= diff_line_content(left.text)
         - else
           %td.old_line.diff-line-num.empty-cell
           %td.line_content.parallel
@@ -38,11 +40,12 @@
             - right_line_code = diff_file.line_code(right)
             - right_position = diff_file.position(right)
             %td.new_line.diff-line-num.js-avatar-container{ id: right_line_code, class: right.type, data: { linenumber: right.new_pos } }
+              = add_diff_note_button(right_line_code, right_position, 'new')
               %a{ href: "##{right_line_code}", data: { linenumber: right.new_pos } }
               - discussion_right = discussions_right.try(:first)
               - if discussion_right && discussion_right.resolvable?
                 %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)
+            %td.line_content.parallel.noteable_line{ class: right.type }= diff_line_content(right.text)
         - else
           %td.old_line.diff-line-num.empty-cell
           %td.line_content.parallel

features/steps/shared/diff_note.rb

@@ -232,7 +232,7 @@ module SharedDiffNote
   end
 
   def click_parallel_diff_line(code, line_type)
-    find(".line_content.parallel.#{line_type}[data-line-code='#{code}']").trigger 'mouseover'
+    find(".line_holder.parallel .diff-line-num[id='#{code}']").trigger 'mouseover'
     find(".line_holder.parallel button[data-line-code='#{code}']").trigger 'click'
   end
 end

spec/features/expand_collapse_diffs_spec.rb

@@ -129,7 +129,7 @@ feature 'Expand and collapse diffs', js: true, feature: true do
 
         before do
           large_diff.find('.diff-line-num', match: :prefer_exact).hover
-          large_diff.find('.add-diff-note').click
+          large_diff.find('.add-diff-note', match: :prefer_exact).click
           large_diff.find('.note-textarea').send_keys comment_text
           large_diff.find_button('Comment').click
           wait_for_requests