Improved the diff comment button UX

It now shows the line will be commenting on my highlight the line number cells with a lighter color. The button has also been made smaller, it was previously way over the top & took over a lot more space than it should of done

Closes #27543

9 files changed, 106 insertions, 59 deletions

diff --git a/app/assets/javascripts/files_comment_button.js b/app/assets/javascripts/files_comment_button.js
index 698870d0ce1..651032ea077 100644
--- a/app/assets/javascripts/files_comment_button.js
+++ b/app/assets/javascripts/files_comment_button.js
@@ -2,15 +2,14 @@
 /* global FilesCommentButton */
 
 (function() {
+  var $commentButtonTemplate;
   var bind = function(fn, me) { return function() { return fn.apply(me, arguments); }; };
 
   this.FilesCommentButton = (function() {
-    var COMMENT_BUTTON_CLASS, COMMENT_BUTTON_TEMPLATE, DEBOUNCE_TIMEOUT_DURATION, EMPTY_CELL_CLASS, LINE_COLUMN_CLASSES, LINE_CONTENT_CLASS, LINE_HOLDER_CLASS, LINE_NUMBER_CLASS, OLD_LINE_CLASS, TEXT_FILE_SELECTOR, UNFOLDABLE_LINE_CLASS;
+    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';
 
-    COMMENT_BUTTON_TEMPLATE = _.template('<button name="button" type="submit" class="btn <%- COMMENT_BUTTON_CLASS %> js-add-diff-note-button" title="Add a comment to this line"><i class="fa fa-comment-o"></i></button>');
-
     LINE_HOLDER_CLASS = '.line_holder';
 
     LINE_NUMBER_CLASS = 'diff-line-num';
@@ -27,20 +26,18 @@
 
     TEXT_FILE_SELECTOR = '.text-file';
 
-    DEBOUNCE_TIMEOUT_DURATION = 100;
-
     function FilesCommentButton(filesContainerElement) {
-      var debounce;
       this.filesContainerElement = filesContainerElement;
-      this.destroy = bind(this.destroy, this);
       this.render = bind(this.render, this);
+      this.hideButton = bind(this.hideButton, this);
       this.VIEW_TYPE = $('input#view[type=hidden]').val();
-      debounce = _.debounce(this.render, DEBOUNCE_TIMEOUT_DURATION);
-      $(this.filesContainerElement).off('mouseover', LINE_COLUMN_CLASSES).off('mouseleave', LINE_COLUMN_CLASSES).on('mouseover', LINE_COLUMN_CLASSES, debounce).on('mouseleave', LINE_COLUMN_CLASSES, this.destroy);
+      $(this.filesContainerElement)
+        .on('mouseover', LINE_COLUMN_CLASSES, this.render)
+        .on('mouseleave', LINE_COLUMN_CLASSES, this.hideButton);
     }
 
     FilesCommentButton.prototype.render = function(e) {
-      var $currentTarget, buttonParentElement, lineContentElement, textFileElement;
+      var $currentTarget, buttonParentElement, lineContentElement, textFileElement, $button;
       $currentTarget = $(e.currentTarget);
 
       buttonParentElement = this.getButtonParent($currentTarget);
@@ -48,6 +45,14 @@
       lineContentElement = this.getLineContent($currentTarget);
       if (!this.validateLineContent(lineContentElement)) return;
 
+      $button = $(COMMENT_BUTTON_CLASS, buttonParentElement);
+      buttonParentElement.addClass('is-over')
+        .nextUntil('.line_content').addClass('is-over');
+
+      if ($button.length) {
+        return;
+      }
+
       textFileElement = this.getTextFileElement($currentTarget);
       buttonParentElement.append(this.buildButton({
         noteableType: textFileElement.attr('data-noteable-type'),
@@ -61,19 +66,16 @@
       }));
     };
 
-    FilesCommentButton.prototype.destroy = function(e) {
-      if (this.isMovingToSameType(e)) {
-        return;
-      }
-      $(COMMENT_BUTTON_CLASS, this.getButtonParent($(e.currentTarget))).remove();
+    FilesCommentButton.prototype.hideButton = function(e) {
+      var $currentTarget = $(e.currentTarget);
+      var buttonParentElement = this.getButtonParent($currentTarget);
+
+      buttonParentElement.removeClass('is-over')
+        .nextUntil('.line_content').removeClass('is-over');
     };
 
     FilesCommentButton.prototype.buildButton = function(buttonAttributes) {
-      var initializedButtonTemplate;
-      initializedButtonTemplate = COMMENT_BUTTON_TEMPLATE({
-        COMMENT_BUTTON_CLASS: COMMENT_BUTTON_CLASS.substr(1)
-      });
-      return $(initializedButtonTemplate).attr({
+      return $commentButtonTemplate.clone().attr({
         'data-noteable-type': buttonAttributes.noteableType,
         'data-noteable-id': buttonAttributes.noteableID,
         'data-commit-id': buttonAttributes.commitID,
@@ -114,17 +116,8 @@
       }
     };
 
-    FilesCommentButton.prototype.isMovingToSameType = function(e) {
-      var newButtonParent;
-      newButtonParent = this.getButtonParent($(e.toElement));
-      if (!newButtonParent) {
-        return false;
-      }
-      return newButtonParent.is(this.getButtonParent($(e.currentTarget)));
-    };
-
     FilesCommentButton.prototype.validateButtonParent = function(buttonParentElement) {
-      return !buttonParentElement.hasClass(EMPTY_CELL_CLASS) && !buttonParentElement.hasClass(UNFOLDABLE_LINE_CLASS) && $(COMMENT_BUTTON_CLASS, buttonParentElement).length === 0;
+      return !buttonParentElement.hasClass(EMPTY_CELL_CLASS) && !buttonParentElement.hasClass(UNFOLDABLE_LINE_CLASS);
     };
 
     FilesCommentButton.prototype.validateLineContent = function(lineContentElement) {
@@ -135,6 +128,8 @@
   })();
 
   $.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>');
+
     if (!(this && (this.parent().data('can-create-note') != null))) {
       return;
     }
diff --git a/app/assets/stylesheets/highlight/dark.scss b/app/assets/stylesheets/highlight/dark.scss
index 6f2e746d4b0..c26edec8ee3 100644
--- a/app/assets/stylesheets/highlight/dark.scss
+++ b/app/assets/stylesheets/highlight/dark.scss
@@ -139,6 +139,17 @@ $dark-il: #de935f;
       }
     }
 
+    .diff-line-num {
+      &.is-over {
+        background-color: #ded7fc;
+        border-color: darken(#ded7fc, 5%);
+
+        a {
+          color: darken(#ded7fc, 15%);
+        }
+      }
+    }
+
     .line_content.match {
       @include dark-diff-match-line;
     }
diff --git a/app/assets/stylesheets/highlight/monokai.scss b/app/assets/stylesheets/highlight/monokai.scss
index 2144a5f7466..b311e1085c1 100644
--- a/app/assets/stylesheets/highlight/monokai.scss
+++ b/app/assets/stylesheets/highlight/monokai.scss
@@ -139,6 +139,17 @@ $monokai-gi: #a6e22e;
       }
     }
 
+    .diff-line-num {
+      &.is-over {
+        background-color: #9f9ab5;
+        border-color: darken(#9f9ab5, 5%);
+
+        a {
+          color: darken(#9f9ab5, 15%);
+        }
+      }
+    }
+
     .line_content.match {
       @include dark-diff-match-line;
     }
diff --git a/app/assets/stylesheets/highlight/solarized_dark.scss b/app/assets/stylesheets/highlight/solarized_dark.scss
index 2cb1d18f12f..b238b66c42d 100644
--- a/app/assets/stylesheets/highlight/solarized_dark.scss
+++ b/app/assets/stylesheets/highlight/solarized_dark.scss
@@ -143,6 +143,17 @@ $solarized-dark-il: #2aa198;
       }
     }
 
+    .diff-line-num {
+      &.is-over {
+        background-color: #9f9ab5;
+        border-color: darken(#9f9ab5, 5%);
+
+        a {
+          color: darken(#9f9ab5, 15%);
+        }
+      }
+    }
+
     .line_content.match {
       @include dark-diff-match-line;
     }
diff --git a/app/assets/stylesheets/highlight/solarized_light.scss b/app/assets/stylesheets/highlight/solarized_light.scss
index b72c4326730..0ac1e46a9f7 100644
--- a/app/assets/stylesheets/highlight/solarized_light.scss
+++ b/app/assets/stylesheets/highlight/solarized_light.scss
@@ -150,6 +150,17 @@ $solarized-light-il: #2aa198;
       }
     }
 
+    .diff-line-num {
+      &.is-over {
+        background-color: #ded7fc;
+        border-color: darken(#ded7fc, 5%);
+
+        a {
+          color: darken(#ded7fc, 15%);
+        }
+      }
+    }
+
     .line_content.match {
       @include matchLine;
     }
diff --git a/app/assets/stylesheets/highlight/white.scss b/app/assets/stylesheets/highlight/white.scss
index 398fbfd3b18..9b19282dd6f 100644
--- a/app/assets/stylesheets/highlight/white.scss
+++ b/app/assets/stylesheets/highlight/white.scss
@@ -123,6 +123,15 @@ $white-gc-bg: #eaf2f5;
         }
       }
 
+      &.is-over {
+        background-color: #ded7fc;
+        border-color: darken(#ded7fc, 5%);
+
+        a {
+          color: darken(#ded7fc, 15%);
+        }
+      }
+
       &.hll:not(.empty-cell) {
         background-color: $line-number-select;
         border-color: $line-select-yellow-dark;
diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss
index 92d7772da57..a8704c96011 100644
--- a/app/assets/stylesheets/pages/diff.scss
+++ b/app/assets/stylesheets/pages/diff.scss
@@ -109,10 +109,6 @@
       td.line_content.parallel {
         width: 46%;
       }
-
-      .add-diff-note {
-        margin-left: -65px;
-      }
     }
 
     .old_line,
diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index aa130a1abb0..692925c8338 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -452,36 +452,35 @@ ul.notes {
  * Line note button on the side of diffs
  */
 
-.diff-file tr.line_holder {
-  @mixin show-add-diff-note {
-    display: inline-block;
-  }
+.add-diff-note {
+  display: none;
+  margin-top: -2px;
+  border-radius: 50%;
+  background: $white-light;
+  padding: 2px 5px;
+  font-size: 12px;
+  color: $gl-link-color;
+  margin-left: -55px;
+  position: absolute;
+  z-index: 10;
+  width: 23px;
+  height: 23px;
+  border: 1px solid $border-color;
 
-  .add-diff-note {
-    margin-top: -8px;
-    border-radius: 40px;
-    background: $white-light;
-    padding: 4px;
-    font-size: 16px;
-    color: $gl-link-color;
-    margin-left: -56px;
-    position: absolute;
-    z-index: 10;
-    width: 32px;
-    // "hide" it by default
-    display: none;
+  &:hover {
+    background: $gl-info;
+    color: $white-light;
+  }
 
-    &:hover {
-      background: $gl-info;
-      color: $white-light;
-      @include show-add-diff-note;
-    }
+  &:active {
+    outline: 0;
   }
+}
 
-  // "show" the icon also if we just hover somewhere over the line
-  &:hover > td {
+.diff-file {
+  .is-over {
     .add-diff-note {
-      @include show-add-diff-note;
+      display: inline-block;
     }
   }
 }
diff --git a/changelogs/unreleased/mr-diff-comment-button.yml b/changelogs/unreleased/mr-diff-comment-button.yml
new file mode 100644
index 00000000000..1dc6ed1c495
--- /dev/null
+++ b/changelogs/unreleased/mr-diff-comment-button.yml
@@ -0,0 +1,4 @@
+---
+title: Improved diff comment button UX
+merge_request:
+author: