Refactor diff notes creation

app/assets/javascripts/notes.js

@@ -17,34 +17,6 @@ var NoteList = {
       this.reversed = $("#notes-list").is(".reversed");
       this.target_params = "target_type=" + this.target_type + "&target_id=" + this.target_id;
 
-      // get initial set of notes
-      this.getContent();
-
-      $("#notes-list, #new-notes-list").on("ajax:success", ".js-note-delete", function() {
-        $(this).closest('li').fadeOut(function() {
-          $(this).remove();
-          NoteList.updateVotes();
-        });
-      });
-
-      $(".note-form-holder").on("ajax:before", function(){
-        $(".submit_note").disable();
-      })
-
-      $(".note-form-holder").on("ajax:complete", function(){
-        $(".submit_note").enable();
-        $('#preview-note').hide();
-        $('#note_note').show();
-      })
-
-      disableButtonIfEmptyField(".note-text", ".submit_note");
-
-      $("#note_attachment").change(function(e){
-        var val = $('.input-file').val();
-        var filename = val.replace(/^.*[\\\/]/, '');
-        $(".file_name").text(filename);
-      });
-
       if(this.reversed) {
         var textarea = $(".note-text");
         $('.note_advanced_opts').hide();
@@ -55,6 +27,17 @@ var NoteList = {
         });
       }
 
+      // get initial set of notes
+      this.getContent();
+
+      disableButtonIfEmptyField(".js-note-text", ".js-comment-button");
+
+      $("#note_attachment").change(function(e){
+        var val = $('.input-file').val();
+        var filename = val.replace(/^.*[\\\/]/, '');
+        $(".file_name").text(filename);
+      });
+
       // Setup note preview
       $(document).on('click', '#preview-link', function(e) {
         $('#preview-note').text('Loading...');
@@ -72,11 +55,170 @@ var NoteList = {
         }
 
         $('#preview-note, #note_note').toggle();
-        e.preventDefault();
+      });+
+
+      $(document).on("click",
+                     ".js-add-diff-note-button",
+                     NoteList.addDiffNote);
+
+      // reply to diff notes
+      $(document).on("click",
+                     ".js-diff-note-reply-button",
+                     NoteList.replyToDiffNote);
+
+      // hide diff note form
+      $(document).on("click",
+                     ".js-hide-diff-note-form",
+                     NoteList.removeDiffNoteForm);
+
+      // do some diff note specific housekeeping when removing a diff note
+      $(document).on("click",
+                     ".diff_file .js-note-delete",
+                     NoteList.removeDiffNote);
+
+      // remove a note (in general)
+      $(document).on("click",
+                     ".js-note-delete",
+                     NoteList.removeNote);
+
+      // clean up previews for forms
+      $(document).on("ajax:complete", ".note-form-holder", function(){
+        $(this).find('#preview-note').hide();
+        $(this).find('#note_note').show();
       });
     },
 
 
+  /**
+   * Event handlers
+   */
+
+
+  /**
+   * Called when clicking on the "add a comment" button on the side of a diff line.
+   *
+   * Inserts a temporary row for the form below the line.
+   * Sets up the form and shows it.
+   */
+  addDiffNote: function(e) {
+    // find the form
+    var form = $(".js-note-forms .js-diff-note-form");
+    var row = $(this).closest("tr");
+    var nextRow = row.next();
+
+    // does it already have notes?
+    if (nextRow.is(".notes_holder")) {
+      $.proxy(NoteList.replyToDiffNote,
+              nextRow.find(".js-diff-note-reply-button")
+             ).call();
+    } else {
+      // add a notes row and insert the form
+      row.after('<tr class="notes_holder js-temp-notes-holder"><td class="notes_line" colspan="2"></td><td class="notes_content"></td></tr>');
+      form.clone().appendTo(row.next().find(".notes_content"));
+
+      // show the form
+      NoteList.setupDiffNoteForm($(this), row.next().find("form"));
+    }
+
+    e.preventDefault();
+  },
+
+  /**
+   * Called in response to deleting a note on a diff line.
+   *
+   * Removes the actual note from view.
+   * Removes the whole notes row if the last note for that line is being removed.
+   *
+   * Note: must be called before removeNote()
+   */
+  removeDiffNote: function() {
+    var notes = $(this).closest(".notes");
+
+    // check if this is the last note for this line
+    if (notes.find(".note").length === 1) {
+      notes.closest("tr").remove();
+    }
+  },
+
+  /**
+   * Called in response to "cancel" on a diff note form.
+   * 
+   * Shows the reply button again.
+   * Removes the form and if necessary it's temporary row.
+   */
+  removeDiffNoteForm: function(e) {
+    var form = $(this).closest("form");
+    var row = form.closest("tr");
+
+    // show the reply button (will only work for replys)
+    form.prev(".js-diff-note-reply-button").show();
+
+    if (row.is(".js-temp-notes-holder")) {
+      // remove temporary row
+      row.remove();
+    } else {
+      // only remove the form
+      form.remove();
+    }
+
+    e.preventDefault();
+  },
+
+  /**
+   * Called in response to deleting a note of any kind.
+   *
+   * Removes the actual note from view.
+   */
+  removeNote: function() {
+    $(this).closest(".note").remove();
+    NoteList.updateVotes();
+  },
+
+  /**
+   * Called when clicking on the "reply" button for a diff line.
+   *
+   * Shows the note form below the notes.
+   */
+  replyToDiffNote: function() {
+    // find the form
+    var form = $(".js-note-forms .js-diff-note-form");
+
+
+    // hide reply button
+    $(this).hide();
+    // insert the form after the button
+    form.clone().insertAfter($(this));
+
+    // show the form
+    NoteList.setupDiffNoteForm($(this), $(this).next("form"));
+  },
+
+  /**
+   * Shows the diff line form and does some setup.
+   *
+   * Sets some hidden fields in the form.
+   *
+   * Note: "this" must have the "discussionId", "lineCode", "noteableType" and
+   *       "noteableId" data attributes set.
+   */
+  setupDiffNoteForm: function(data_holder, form) {
+    // setup note target
+    form.attr("rel", data_holder.data("discussionId"));
+    form.find("#note_line_code").val(data_holder.data("lineCode"));
+    form.find("#note_noteable_type").val(data_holder.data("noteableType"));
+    form.find("#note_noteable_id").val(data_holder.data("noteableId"));
+
+    // setup interaction
+    disableButtonIfEmptyField(form.find(".js-note-text"), form.find(".js-comment-button"));
+    setupGfmAutoComplete();
+
+    // cleanup after successfully creating a diff note
+    form.on("ajax:success", NoteList.removeDiffNoteForm);
+
+    form.show();
+  },
+
+
   /**
    * Handle loading the initial set of notes.
    * And set up loading more notes when scrolling to the bottom of the page.
@@ -272,52 +414,3 @@ var NoteList = {
       }
     }
 };
-
-var PerLineNotes = {
-  init:
-    function() {
-      $(".per_line_form .hide-button").on("click", function(){
-        $(this).closest(".per_line_form").hide();
-        return false;
-      });
-
-      /**
-       * Called when clicking on the "add note" or "reply" button for a diff line.
-       *
-       * Shows the note form below the line.
-       * Sets some hidden fields in the form.
-       */
-      $(".diff_file_content").on("click", ".js-note-add-to-diff-line", function(e) {
-        var form = $(".per_line_form");
-        $(this).closest("tr").after(form);
-        form.find("#note_line_code").val($(this).data("lineCode"));
-        form.find("#note_noteable_type").val($(this).data("noteableType"));
-        form.find("#note_noteable_id").val($(this).data("noteableId"));
-        form.show();
-        e.preventDefault();
-      });
-
-      disableButtonIfEmptyField(".line-note-text", ".submit_inline_note");
-
-      /**
-       * Called in response to successfully deleting a note on a diff line.
-       *
-       * Removes the actual note from view.
-       * Removes the reply button if the last note for that line has been removed.
-       */
-      $(".diff_file_content").on("ajax:success", ".js-note-delete", function() {
-        var trNote = $(this).closest("tr");
-        trNote.fadeOut(function() {
-          $(this).remove();
-        });
-
-        // check if this is the last note for this line
-        // elements must really be removed for this to work reliably
-        var trLine = trNote.prev();
-        var trRpl  = trNote.next();
-        if (trLine.is(".line_holder") && trRpl.is(".reply")) {
-          trRpl.fadeOut(function() { $(this).remove(); });
-        }
-      });
-    }
-}

app/assets/stylesheets/sections/commits.scss

@@ -194,12 +194,6 @@
     -khtml-user-select: none;
     user-select: none;
 
-    &.notes_line {
-      border: 1px solid #ccc;
-      border-left: none;
-      text-align: center;
-      padding: 10px 0;
-    }
     a {
       float: left;
       width: 35px;
@@ -227,10 +221,6 @@
       background: #fafafa;
     }
   }
-  .notes_content {
-    border: 1px solid #ccc;
-    border-width: 1px 0;
-  }
 }
 
 /** COMMIT BLOCK **/

app/assets/stylesheets/sections/notes.scss

@@ -92,23 +92,58 @@ ul.notes {
   }
 }
 
-.comment-btn {
-  @extend .save-btn;
-}
-
 .diff_file tr.notes_holder {
   font-family: $sansFontFamily;
   font-size: 13px;
   line-height: 18px;
 
-  td:last-child {
+  td {
+    border: 1px solid #ddd;
+    border-left: none;
+
+    &.notes_line {
+      text-align: center;
+      padding: 10px 0;
+    }
+    &.notes_content {
       background-color: $white;
+      border-width: 1px 0;
       padding-top: 0;
     }
+  }
 
   .comment-btn {
     margin-top: 8px;
   }
+
+  // TODO: start cleanup
+  form {
+    // hide it by default
+    display: none;
+    margin: 8px 0;
+
+    .note_actions {
+      margin:0;
+      padding-top: 10px;
+
+      .buttons {
+        float:left;
+        width:300px;
+      }
+      .options {
+        .labels {
+          float:left;
+          padding-left:10px;
+          label {
+            padding: 6px 0;
+            margin: 0;
+            width:120px;
+          }
+        }
+      }
+    }
+  }
+  // TODO: end cleanup
 }
 
 /**
@@ -158,74 +193,6 @@ ul.notes {
   }
 }
 
-/*
- * New Note Form
- */
-.new_note {
-  /* Note textare */
-  #note_note {
-    height:80px;
-    width:99%;
-    font-size:14px;
-  }
-}
-
-
-#new_note {
-  .attach_holder {
-    display: none;
-  }
-}
-
-.preview_note {
-  margin: 2px;
-  border: 1px solid #ddd;
-  padding: 10px;
-  min-height: 60px;
-  background: #f5f5f5;
-}
-
-.note {
-  padding: 8px 0;
-  overflow: hidden;
-  display: block;
-  position: relative;
-  img {float: left; margin-right: 10px;}
-  img.emoji {float: none;margin: 0;}
-  .note-author cite{font-style: italic;}
-  p { color: $style_color; }
-  .note-author { color: $style_color;}
-
-  .note-title { margin-left: 45px; padding-top: 5px;}
-  .avatar {
-    margin-top: 3px;
-  }
-
-  .delete-note {
-    display: none;
-    position: absolute;
-    right: 0;
-    top: 0;
-  }
-
-  &:hover {
-    .delete-note { display: block; }
-  }
-}
-#notes-list:not(.reversed) .note,
-#new-notes-list:not(.reversed) .note {
-  border-bottom: 1px solid #eee;
-}
-#notes-list.reversed .note,
-#new-notes-list.reversed .note {
-  border-top: 1px solid #eee;
-}
-
-/* mark vote notes */
-.voting_notes .note {
-  padding: 8px 0;
-}
-
 .notes-status {
   margin: 18px;
 }
@@ -239,45 +206,18 @@ p.notify_controls span{
   font-weight: 700;
 }
 
-.line_notes_row,  .per_line_form { font-family: "Helvetica Neue", Helvetica, Arial, sans-serif; }
-
-.per_line_form {
-  background: #f5f5f5;
-  border-top: 1px solid #eee;
-  form {  margin: 0; }
-  td {
-    border-bottom: 1px solid #ddd;
-  }
-  .note_actions {
-    margin: 0;
-    padding-top: 10px;
-
-    .buttons {
-      float: left;
-      width: 300px;
-    }
-    .options {
-      .labels {
-        float: left;
-        padding-left: 10px;
-        label {
-          padding: 6px 0;
-          margin: 0;
-          width: 120px;
-        }
-      }
-    }
-  }
-}
-
-td .line_note_link {
-  position: absolute;
+/**
+ * add line note button on the side of diffs
+ */
+.diff_file tr.line_holder {
+  .add-diff-note {
+    position:absolute;
     margin-left:-70px;
     margin-top:-10px;
-  z-index: 10;
+    z-index:10;
     background: url("comment_add.png") no-repeat left 0;
-  width: 32px;
-  height: 32px;
+    width:32px;
+    height:32px;
 
     opacity: 0.0;
     filter: alpha(opacity=0);
@@ -286,15 +226,54 @@ td .line_note_link {
       opacity: 1.0;
       filter: alpha(opacity=100);
     }
-}
+  }
 
-.diff_file_content tr.line_holder:hover > td { background: $hover !important; }
-.diff_file_content tr.line_holder:hover > td .line_note_link {
+  &:hover > td {
+    background: $hover !important;
+
+    .add-diff-note {
       opacity: 1.0;
       filter: alpha(opacity=100);
+    }
+  }
 }
 
+/**
+ * Note Forms
+ */
+
+.comment-btn {
+  @extend .save-btn;
+}
 .new_note {
+  textarea {
+    height:80px;
+    width:99%;
+    font-size:14px;
+  }
+}
+.note-forms {
+  .new_diff_note {
+    display: none;
+  }
+}
+
+
+#new_note {
+  .attach_holder {
+    display:none;
+  }
+}
+
+.preview_note {
+  margin: 2px;
+  border: 1px solid #ddd;
+  padding: 10px;
+  min-height: 60px;
+  background:#f5f5f5;
+}
+
+form.new_note {
   .input-file {
     font: 500px monospace;
     opacity: 0;

app/controllers/notes_controller.rb

@@ -12,11 +12,8 @@ class NotesController < ProjectResourceController
     @notes = Notes::LoadContext.new(project, current_user, params).execute
 
     if params[:target_type] == "merge_request"
-      @has_diff      = true
       @mixed_targets = true
       @discussions   = discussions_from_notes
-    elsif params[:target_type] == "commit"
-      @has_diff = true
     end
 
     respond_with(@notes)

app/views/commit/show.html.haml

 = render "commits/commit_box"
 = render "commits/diffs", diffs: @commit.diffs
 = render "notes/notes_with_form", tid: @commit.id, tt: "commit"
-= render "notes/diff_note_form"
-
 
 :javascript
   $(function(){
-    PerLineNotes.init();
     var w, h;
     $('.diff_file').each(function(){
       $('.image.diff_removed img', this).on('load', $.proxy(function(event){
@@ -19,7 +16,5 @@
           , h = event.currentTarget.naturalHeight;
         $('.image.diff_added .image-info', this).append(' | <b>W:</b> ' + w + 'px | <b>H:</b> ' + h + 'px');
       }, this));
-      
     });
-    
   });

app/views/merge_requests/_show.html.haml

@@ -21,8 +21,6 @@
   = render "merge_requests/show/diffs" if @diffs
 .status
 
-= render "notes/diff_note_form"
-
 :javascript
   $(function(){
     MergeRequest.init({

app/views/merge_requests/diffs.js.haml

 :plain
   $(".merge-request-diffs").html("#{escape_javascript(render(partial: "merge_requests/show/diffs"))}");
 
-  $(function(){
-    PerLineNotes.init();
-  });
-

app/views/notes/_common_form.html.haml

@@ -8,7 +8,7 @@
 
     = f.hidden_field :noteable_id
     = f.hidden_field :noteable_type
-    = f.text_area :note, size: 255, class: 'note-text js-gfm-input'
+    = f.text_area :note, size: 255, class: 'js-note-text js-gfm-input'
     #preview-note.preview_note.hide
     .hint
       .right Comments are parsed with #{link_to "GitLab Flavored Markdown", help_markdown_path, target: '_blank'}.
@@ -16,7 +16,7 @@
 
     .row.note_advanced_opts
       .span3
-        = f.submit 'Add Comment', class: "btn comment-btn submit_note grouped", id: "submit_note"
+        = f.submit 'Add Comment', class: "btn comment-btn grouped js-comment-button"
         = link_to 'Preview', preview_project_notes_path(@project), class: 'btn grouped', id: 'preview-link'
       .span4.notify_opts
         %h6.left Notify via email:

app/views/notes/_create_diff_note.js.haml

 - if note.valid?
   :plain
     // hide and reset the form
-    $(".per_line_form").hide();
-    $('.line-note-form-holder textarea').val("");
+    var form = $("form[rel='#{note.discussion_id}']");
+    var row = form.closest("tr");
 
-    // find the reply button for this line
-    // (might not be there if this is the first note)
-    var trRpl = $(".js-note-add-to-diff-line[data-noteable-type='#{note.noteable_type}'][data-noteable-id='#{note.noteable_id}'][data-line-code='#{note.line_code}']").closest("tr");
-    if (trRpl.size() == 0) {
-      // find the commented line ...
-      var trEl = $(".#{note.line_code}").parent();
-      // ... and insert the note and the reply button after it
-      trEl.after("#{escape_javascript(render "notes/diff_notes_with_reply", notes: [note])}");
+    // is this the first note?
+    if (row.is(".js-temp-notes-holder")) {
+      // insert the note and the reply button after it
+      row.after("#{escape_javascript(render "notes/diff_notes_with_reply", notes: [note])}");
     } else {
       // instert new note before reply button
-      trRpl.before("#{escape_javascript(render "notes/diff_note", note: note)}");
+      row.find(".notes").append("#{escape_javascript(render "notes/note", note: note)}");
     }

app/views/notes/_diff_note_form.html.haml

-%table{style: "display:none;"}
-  %tr.per_line_form
-    %td{colspan: 3 }
-      .line-note-form-holder
-        = form_for [@project, @note], remote: "true", multipart: true do |f|
-          %h3.page_title Leave a comment
-          %div.span10
+= form_for [@project, @note], remote: true, html: { multipart: true, class: "new_note new_diff_note js-diff-note-form" } do |f|
+  .span10
     -if @note.errors.any?
       .alert-message.block-message.error
         - @note.errors.full_messages.each do |msg|
@@ -13,11 +8,11 @@
     = f.hidden_field :noteable_id
     = f.hidden_field :noteable_type
     = f.hidden_field :line_code
-            = f.text_area :note,  size: 255, class: 'line-note-text js-gfm-input'
+    = f.text_area :note,  size: 255, class: 'js-note-text js-gfm-input'
     .note_actions
       .buttons
-                = f.submit 'Add Comment', class: "btn save-btn submit_note submit_inline_note", id: "submit_note"
-                = link_to "Cancel", "javascript:;", class: "btn hide-button"
+        = f.submit 'Add Comment', class: "btn save-btn js-comment-button"
+        %button.btn.js-hide-diff-note-form Cancel
       .options
         %h6.left Notify via email:
         .labels

app/views/notes/_diff_note_link.html.haml

+- note = @project.notes.new(@comments_target.merge({ line_code: line_code }))
 = link_to "",
-          "#",
-          id: "add-diff-line-note-#{line_code}",
-          class: "line_note_link js-note-add-to-diff-line",
-          data: @comments_target.merge({ line_code: line_code }),
+          "javascript:;",
+          class: "add-diff-note js-add-diff-note-button",
+          data: { noteable_type: note.noteable_type,
+                  noteable_id:   note.noteable_id,
+                  line_code:     note.line_code,
+                  discussion_id: note.discussion_id },
           title: "Add a comment to this line"

app/views/notes/_diff_notes_with_reply.html.haml

-%tr.notes_holder
+- note = notes.first # example note
+%tr.notes_holder{ data: { :'discussion-id' => note.discussion_id } }
   %td.notes_line{ colspan: 2 }
     %span.btn.disabled
       %i.icon-comment
@@ -8,11 +9,12 @@
       = render notes
 
     -# reply button
-    - note = notes.first # example note
-    %button{  class: "btn comment-btn js-note-add-to-diff-line",
-              data: { line_code:     note.line_code,
-                      noteable_type: note.noteable_type,
-                      noteable_id:   note.noteable_id },
-              title: "Add a comment to this line" }
+    = link_to "javascript:;",
+              class: "btn comment-btn js-diff-note-reply-button",
+              data: { noteable_type: note.noteable_type,
+                      noteable_id:   note.noteable_id,
+                      line_code:     note.line_code,
+                      discussion_id: note.discussion_id },
+              title: "Add a comment to this line" do
       %i.icon-comment
       Reply

app/views/notes/_notes_with_form.html.haml

@@ -3,7 +3,9 @@
 .notes-status
 
 - if can? current_user, :write_note, @project
+  .note-forms.js-note-forms
     = render "notes/common_form"
+    = render "notes/diff_note_form"
 
 :javascript
   $(function(){

app/views/notes/create.js.haml

@@ -2,7 +2,3 @@
   = render "create_diff_note", note: @note
 - else
   = render "create_common_note", note: @note
-
--# Enable submit button
-:plain
-  $("#submit_note").removeAttr("disabled");

app/views/notes/index.js.haml

@@ -16,7 +16,3 @@
   - if loading_more_notes?
     :plain
       NoteList.finishedLoadingMore();
-
-- if @has_diff
-  :plain
-    PerLineNotes.init();