Merge branch 'mr-diff-comment-button' into 'master'

Improved the diff comment button UX

Closes #27543

See merge request !9448

9 files changed, 129 insertions, 68 deletions

diff --git a/app/assets/javascripts/files_comment_button.js b/app/assets/javascripts/files_comment_button.js
index 698870d0ce1..6d86888dcb8 100644
--- a/app/assets/javascripts/files_comment_button.js
+++ b/app/assets/javascripts/files_comment_button.js
@@ -1,16 +1,16 @@
 /* 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 */
 
 (function() {
+  let $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,26 +27,29 @@
 
     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.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.hideButton = bind(this.hideButton, this);
+      this.isParallelView = notes.isParallelView();
+      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);
-      if (!this.validateButtonParent(buttonParentElement)) return;
       lineContentElement = this.getLineContent($currentTarget);
-      if (!this.validateLineContent(lineContentElement)) return;
+      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;
+      }
 
       textFileElement = this.getTextFileElement($currentTarget);
       buttonParentElement.append(this.buildButton({
@@ -61,19 +64,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_CLASS}`).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,
@@ -86,14 +86,14 @@
     };
 
     FilesCommentButton.prototype.getTextFileElement = function(hoveredElement) {
-      return $(hoveredElement.closest(TEXT_FILE_SELECTOR));
+      return hoveredElement.closest(TEXT_FILE_SELECTOR);
     };
 
     FilesCommentButton.prototype.getLineContent = function(hoveredElement) {
       if (hoveredElement.hasClass(LINE_CONTENT_CLASS)) {
         return hoveredElement;
       }
-      if (this.VIEW_TYPE === 'inline') {
+      if (!this.isParallelView) {
         return $(hoveredElement).closest(LINE_HOLDER_CLASS).find("." + LINE_CONTENT_CLASS);
       } else {
         return $(hoveredElement).next("." + LINE_CONTENT_CLASS);
@@ -101,7 +101,7 @@
     };
 
     FilesCommentButton.prototype.getButtonParent = function(hoveredElement) {
-      if (this.VIEW_TYPE === 'inline') {
+      if (!this.isParallelView) {
         if (hoveredElement.hasClass(OLD_LINE_CLASS)) {
           return hoveredElement;
         }
@@ -114,17 +114,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 +126,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..9c76e58dfc8 100644
--- a/app/assets/stylesheets/highlight/dark.scss
+++ b/app/assets/stylesheets/highlight/dark.scss
@@ -20,6 +20,7 @@ $dark-highlight-bg: #ffe792;
 $dark-highlight-color: $black;
 $dark-pre-hll-bg: #373b41;
 $dark-hll-bg: #373b41;
+$dark-over-bg: #9f9ab5;
 $dark-c: #969896;
 $dark-err: #c66;
 $dark-k: #b294bb;
@@ -139,6 +140,18 @@ $dark-il: #de935f;
       }
     }
 
+    .diff-line-num {
+      &.is-over,
+      &.hll:not(.empty-cell).is-over {
+        background-color: $dark-over-bg;
+        border-color: darken($dark-over-bg, 5%);
+
+        a {
+          color: darken($dark-over-bg, 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..6488a099c74 100644
--- a/app/assets/stylesheets/highlight/monokai.scss
+++ b/app/assets/stylesheets/highlight/monokai.scss
@@ -13,6 +13,7 @@ $monokai-line-empty-bg: #49483e;
 $monokai-line-empty-border: darken($monokai-line-empty-bg, 15%);
 $monokai-diff-border: #808080;
 $monokai-highlight-bg: #ffe792;
+$monokai-over-bg: #9f9ab5;
 
 $monokai-new-bg: rgba(166, 226, 46, 0.1);
 $monokai-new-idiff: rgba(166, 226, 46, 0.15);
@@ -139,6 +140,18 @@ $monokai-gi: #a6e22e;
       }
     }
 
+    .diff-line-num {
+      &.is-over,
+      &.hll:not(.empty-cell).is-over {
+        background-color: $monokai-over-bg;
+        border-color: darken($monokai-over-bg, 5%);
+
+        a {
+          color: darken($monokai-over-bg, 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..00079cc2b5b 100644
--- a/app/assets/stylesheets/highlight/solarized_dark.scss
+++ b/app/assets/stylesheets/highlight/solarized_dark.scss
@@ -17,6 +17,7 @@ $solarized-dark-line-color-new: #5a766c;
 $solarized-dark-line-color-old: #7a6c71;
 $solarized-dark-highlight: #094554;
 $solarized-dark-hll-bg: #174652;
+$solarized-dark-over-bg: #9f9ab5;
 $solarized-dark-c: #586e75;
 $solarized-dark-err: #93a1a1;
 $solarized-dark-g: #93a1a1;
@@ -143,6 +144,18 @@ $solarized-dark-il: #2aa198;
       }
     }
 
+    .diff-line-num {
+      &.is-over,
+      &.hll:not(.empty-cell).is-over {
+        background-color: $solarized-dark-over-bg;
+        border-color: darken($solarized-dark-over-bg, 5%);
+
+        a {
+          color: darken($solarized-dark-over-bg, 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..2e3b68f1a58 100644
--- a/app/assets/stylesheets/highlight/solarized_light.scss
+++ b/app/assets/stylesheets/highlight/solarized_light.scss
@@ -18,6 +18,7 @@ $solarized-light-line-color-new: #a1a080;
 $solarized-light-line-color-old: #ad9186;
 $solarized-light-highlight: #eee8d5;
 $solarized-light-hll-bg: #ddd8c5;
+$solarized-light-over-bg: #ded7fc;
 $solarized-light-c: #93a1a1;
 $solarized-light-err: #586e75;
 $solarized-light-g: #586e75;
@@ -150,6 +151,18 @@ $solarized-light-il: #2aa198;
       }
     }
 
+    .diff-line-num {
+      &.is-over,
+      &.hll:not(.empty-cell).is-over {
+        background-color: $solarized-light-over-bg;
+        border-color: darken($solarized-light-over-bg, 5%);
+
+        a {
+          color: darken($solarized-light-over-bg, 15%);
+        }
+      }
+    }
+
     .line_content.match {
       @include matchLine;
     }
diff --git a/app/assets/stylesheets/highlight/white.scss b/app/assets/stylesheets/highlight/white.scss
index 398fbfd3b18..0eca30e570f 100644
--- a/app/assets/stylesheets/highlight/white.scss
+++ b/app/assets/stylesheets/highlight/white.scss
@@ -7,6 +7,7 @@ $white-code-color: $gl-text-color;
 $white-highlight: #fafe3d;
 $white-pre-hll-bg: #f8eec7;
 $white-hll-bg: #f8f8f8;
+$white-over-bg: #ded7fc;
 $white-c: #998;
 $white-err: #a61717;
 $white-err-bg: #e3d2d2;
@@ -123,6 +124,16 @@ $white-gc-bg: #eaf2f5;
         }
       }
 
+      &.is-over,
+      &.hll:not(.empty-cell).is-over {
+        background-color: $white-over-bg;
+        border-color: darken($white-over-bg, 5%);
+
+        a {
+          color: darken($white-over-bg, 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..339cdcde480 100644
--- a/app/assets/stylesheets/pages/diff.scss
+++ b/app/assets/stylesheets/pages/diff.scss
@@ -89,6 +89,10 @@
 
       .diff-line-num {
         width: 50px;
+
+        a {
+          transition: none;
+        }
       }
 
       .line_holder td {
@@ -109,10 +113,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..00f5f2645b3 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -452,36 +452,37 @@ 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: 1px 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;
+  transition: transform .1s ease-in-out;
 
-  .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;
+    transform: scale(1.15);
+  }
 
-    &: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: