Refactor discussion reply

app/assets/javascripts/notes.js

@@ -63,18 +63,19 @@ var NoteList = {
 
       // reply to diff notes
       $(document).on("click",
-                     ".js-diff-note-reply-button",
+                     ".js-discussion-reply-button",
-                     NoteList.replyToDiffNote);
+                     NoteList.replyToDiscussionNote);
 
       // hide diff note form
       $(document).on("click",
-                     ".js-hide-diff-note-form",
+                     ".js-close-discussion-note-form",
-                     NoteList.removeDiffNoteForm);
+                     NoteList.removeDiscussionNoteForm);
 
-      // do some diff note specific housekeeping when removing a diff note
+      // do some specific housekeeping when removing a diff or discussion note
       $(document).on("click",
-                     ".diff_file .js-note-delete",
+                     ".diff_file .js-note-delete," +
-                     NoteList.removeDiffNote);
+                     ".discussion .js-note-delete",
+                     NoteList.removeDiscussionNote);
 
       // remove a note (in general)
       $(document).on("click",
@@ -102,14 +103,14 @@ var NoteList = {
    */
   addDiffNote: function(e) {
     // find the form
-    var form = $(".js-note-forms .js-diff-note-form");
+    var form = $(".js-note-forms .js-discussion-note-form");
     var row = $(this).closest("tr");
     var nextRow = row.next();
 
     // does it already have notes?
     if (nextRow.is(".notes_holder")) {
-      $.proxy(NoteList.replyToDiffNote,
+      $.proxy(NoteList.replyToDiscussionNote,
-              nextRow.find(".js-diff-note-reply-button")
+              nextRow.find(".js-discussion-reply-button")
              ).call();
     } else {
       // add a notes row and insert the form
@@ -117,7 +118,7 @@ var NoteList = {
       form.clone().appendTo(row.next().find(".notes_content"));
 
       // show the form
-      NoteList.setupDiffNoteForm($(this), row.next().find("form"));
+      NoteList.setupDiscussionNoteForm($(this), row.next().find("form"));
     }
 
     e.preventDefault();
@@ -131,11 +132,15 @@ var NoteList = {
    *
    * Note: must be called before removeNote()
    */
-  removeDiffNote: function() {
+  removeDiscussionNote: function() {
     var notes = $(this).closest(".notes");
 
     // check if this is the last note for this line
     if (notes.find(".note").length === 1) {
+      // for discussions
+      notes.closest(".discussion").remove();
+
+      // for diff lines
       notes.closest("tr").remove();
     }
   },
@@ -146,15 +151,15 @@ var NoteList = {
    * Shows the reply button again.
    * Removes the form and if necessary it's temporary row.
    */
-  removeDiffNoteForm: function(e) {
+  removeDiscussionNoteForm: 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();
+    form.prev(".js-discussion-reply-button").show();
 
     if (row.is(".js-temp-notes-holder")) {
-      // remove temporary row
+      // remove temporary row for diff lines
       row.remove();
     } else {
       // only remove the form
@@ -179,10 +184,9 @@ var NoteList = {
    *
    * Shows the note form below the notes.
    */
-  replyToDiffNote: function() {
+  replyToDiscussionNote: function() {
     // find the form
-    var form = $(".js-note-forms .js-diff-note-form");
+    var form = $(".js-note-forms .js-discussion-note-form");
-
 
     // hide reply button
     $(this).hide();
@@ -190,7 +194,7 @@ var NoteList = {
     form.clone().insertAfter($(this));
 
     // show the form
-    NoteList.setupDiffNoteForm($(this), $(this).next("form"));
+    NoteList.setupDiscussionNoteForm($(this), $(this).next("form"));
   },
 
   /**
@@ -201,7 +205,7 @@ var NoteList = {
    * Note: "this" must have the "discussionId", "lineCode", "noteableType" and
    *       "noteableId" data attributes set.
    */
-  setupDiffNoteForm: function(data_holder, form) {
+  setupDiscussionNoteForm: function(data_holder, form) {
     // setup note target
     form.attr("rel", data_holder.data("discussionId"));
     form.find("#note_line_code").val(data_holder.data("lineCode"));
@@ -210,10 +214,10 @@ var NoteList = {
 
     // setup interaction
     disableButtonIfEmptyField(form.find(".js-note-text"), form.find(".js-comment-button"));
-    setupGfmAutoComplete();
+    GitLab.GfmAutoComplete.setup();
 
     // cleanup after successfully creating a diff note
-    form.on("ajax:success", NoteList.removeDiffNoteForm);
+    form.on("ajax:success", NoteList.removeDiscussionNoteForm);
 
     form.show();
   },
@@ -249,11 +253,11 @@ var NoteList = {
       this.bottom_id = newNoteIds.last();
       $("#notes-list").html(html);
 
+      if (this.reversed) {
         // init infinite scrolling
         this.initLoadMore();
 
         // init getting new notes
-      if (this.reversed) {
         this.initRefreshNew();
       }
     },
@@ -377,9 +381,9 @@ var NoteList = {
   appendNewNote:
     function(id, html) {
       if (this.reversed) {
-        $("#new-notes-list").prepend(html);
+        $("#notes-list").prepend(html);
       } else {
-        $("#new-notes-list").append(html);
+        $("#notes-list").append(html);
       }
       this.updateVotes();
     },

app/assets/stylesheets/sections/notes.scss

@@ -46,9 +46,12 @@ ul.notes {
         @extend .borders;
         background-color: #F9F9F9;
       }
-      .diff_file .note {
+      .diff_file .notes {
-        border-bottom: 0px;
+        /* reset */
-        padding: 0px;
+        background: inherit;
+        border: none;
+        @include box-shadow(none);
+
       }
       .discussion-hidden .note {
         @extend .cgray;
@@ -59,6 +62,9 @@ ul.notes {
         border-color: #ddd;
         padding: 8px;
       }
+      .reply-btn {
+        margin-top: 8px;
+      }
     }
   }
 
@@ -92,7 +98,7 @@ ul.notes {
   }
 }
 
-.diff_file tr.notes_holder {
+.diff_file .notes_holder {
   font-family: $sansFontFamily;
   font-size: 13px;
   line-height: 18px;
@@ -112,38 +118,9 @@ ul.notes {
     }
   }
 
-  .comment-btn {
+  .reply-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
 }
 
 /**
@@ -185,6 +162,7 @@ ul.notes {
   top: 0;
 }
 
+// TODO: start cleaup
 .issue_notes,
 .wiki_notes {
   .note_content {
@@ -193,6 +171,7 @@ ul.notes {
   }
 }
 
+/* for loading indicator */
 .notes-status {
   margin: 18px;
 }
@@ -205,6 +184,7 @@ p.notify_controls input{
 p.notify_controls span{
   font-weight: 700;
 }
+// TODO: end cleaup
 
 /**
  * add line note button on the side of diffs
@@ -242,56 +222,49 @@ p.notify_controls span{
  * Note Forms
  */
 
-.comment-btn {
+.comment-btn,
+.reply-btn {
   @extend .save-btn;
 }
+.new_discussion_note {
+  // hide it by default
+  display: none;
+  margin: 8px 5px 8px 0;
+
+  // TODO: start cleanup
+  .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
+}
 .new_note {
   textarea {
     height:80px;
     width:99%;
     font-size:14px;
   }
-}
-.note-forms {
-  .new_diff_note {
-    display: none;
-  }
-}
 
-
+  // TODO: start cleanup
-#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;
-    filter: alpha(opacity=0);
-    position: absolute;
-    z-index: 1;
-    top: 0;
-    right: 0;
-    padding: 0;
-    margin: 0;
-  }
-
-  .note_advanced_opts {
-    h6 {
-      line-height: 32px;
-      padding-right: 15px;
-    }
-  }
 
   .attachments {
     position: relative;
@@ -301,18 +274,17 @@ form.new_note {
     margin:0 0 5px !important;
 
     .input_file {
-      .file_upload {
-        position: absolute;
-        right: 14px;
-        top: 7px;
-      }
-
       .file_name {
         line-height: 30px;
         width: 240px;
         height: 28px;
         overflow: hidden;
       }
+      .file_upload {
+        position: absolute;
+        right:14px;
+        top:7px;
+      }
       .input-file {
         width: 260px;
         height: 41px;
@@ -320,9 +292,41 @@ form.new_note {
       }
     }
   }
+  .input-file {
+    font: 500px monospace;
+    opacity:0;
+    filter: alpha(opacity=0);
+    position: absolute;
+    z-index: 1;
+    top:0;
+    right:0;
+    padding:0;
+    margin: 0;
+  }
+  .note_advanced_opts {
+    h6 {
+      line-height: 32px;
+      padding-right: 15px;
+    }
+  }
+  .note-text {
+    border: 1px solid #aaa;
+    box-shadow:none;
+  }
+  // TODO: end cleanup
 }
 
-.note-text {
+// hide the new discussion note form template
-  border: 1px solid #aaa;
+.note-forms {
-  box-shadow: none;
+  .new_discussion_note {
+    display: none;
+  }
+}
+
+.preview_note {
+  margin: 2px;
+  border: 1px solid #ddd;
+  padding: 10px;
+  min-height: 60px;
+  background:#f5f5f5;
 }

app/controllers/commit_controller.rb

@@ -14,9 +14,13 @@ class CommitController < ProjectResourceController
     git_not_found! unless @commit
 
     @suppress_diff = result[:suppress_diff]
+
     @note        = result[:note]
     @line_notes  = result[:line_notes]
     @notes_count = result[:notes_count]
+    @target_type = :commit
+    @target_id   = @commit.id
+
     @comments_allowed = @reply_allowed = true
     @comments_target  = { noteable_type: 'Commit',
                           noteable_id: @commit.id }

app/controllers/issues_controller.rb

@@ -38,6 +38,8 @@ class IssuesController < ProjectResourceController
 
   def show
     @note = @project.notes.new(noteable: @issue)
+    @target_type = :issue
+    @target_id = @issue.id
 
     respond_to do |format|
       format.html

app/controllers/merge_requests_controller.rb

@@ -21,6 +21,9 @@ class MergeRequestsController < ProjectResourceController
   end
 
   def show
+    @target_type = :merge_request
+    @target_id = @merge_request.id
+
     respond_to do |format|
       format.html
       format.js

app/controllers/notes_controller.rb

@@ -6,13 +6,11 @@ class NotesController < ProjectResourceController
   respond_to :js
 
   def index
-    @target_note = Note.new(noteable_type: params[:target_type].camelize,
-                            noteable_id: params[:target_id])
-    @target = @target_note.noteable
     @notes = Notes::LoadContext.new(project, current_user, params).execute
+    @target_type = params[:target_type].camelize
+    @target_id = params[:target_id]
 
     if params[:target_type] == "merge_request"
-      @mixed_targets = true
       @discussions   = discussions_from_notes
     end
 
@@ -21,6 +19,8 @@ class NotesController < ProjectResourceController
 
   def create
     @note = Notes::CreateContext.new(project, current_user, params).execute
+    @target_type = params[:target_type].camelize
+    @target_id = params[:target_id]
 
     respond_to do |format|
       format.html {redirect_to :back}
@@ -58,7 +58,7 @@ class NotesController < ProjectResourceController
       next if discussion_ids.include?(note.discussion_id)
 
       # don't group notes for the main target
-      if for_main_target?(note)
+      if note_for_main_target?(note)
         discussions << [note]
       else
         discussions << discussion_notes_for(note)
@@ -70,7 +70,7 @@ class NotesController < ProjectResourceController
   end
 
   # Helps to distinguish e.g. commit notes in mr notes list
-  def for_main_target?(note)
+  def note_for_main_target?(note)
-    !@mixed_targets || (@target.class.name == note.noteable_type && !note.for_diff_line?)
+    @target_type.camelize == note.noteable_type && !note.for_diff_line?
   end
 end

app/controllers/projects_controller.rb

@@ -75,7 +75,10 @@ class ProjectsController < ProjectResourceController
 
   def wall
     return render_404 unless @project.wall_enabled
-    @note = Note.new
+
+    @target_type = :wall
+    @target_id = nil
+    @note = @project.notes.new
 
     respond_to do |format|
       format.html

app/controllers/snippets_controller.rb

@@ -50,6 +50,8 @@ class SnippetsController < ProjectResourceController
 
   def show
     @note = @project.notes.new(noteable: @snippet)
+    @target_type = :snippet
+    @target_id = @snippet.id
   end
 
   def destroy

app/helpers/notes_helper.rb

@@ -1,7 +1,12 @@
 module NotesHelper
    # Helps to distinguish e.g. commit notes in mr notes list
   def note_for_main_target?(note)
-    !@mixed_targets || (@target.class.name == note.noteable_type && !note.for_diff_line?)
+    @target_type.camelize == note.noteable_type && !note.for_diff_line?
+  end
+
+  def note_target_fields
+    hidden_field_tag(:target_type, @target_type) +
+    hidden_field_tag(:target_id, @target_id)
   end
 
   def link_to_commit_diff_line_note(note)

app/views/commit/show.html.haml

@@ -1,6 +1,6 @@
 = render "commits/commit_box"
 = render "commits/diffs", diffs: @commit.diffs
-= render "notes/notes_with_form", tid: @commit.id, tt: "commit"
+= render "notes/notes_with_form"
 
 :javascript
   $(function(){

app/views/issues/show.html.haml

@@ -61,4 +61,4 @@
         = markdown @issue.description
 
 
-.issue_notes.voting_notes#notes= render "notes/notes_with_form", tid: @issue.id, tt: "issue"
+.issue_notes.voting_notes#notes= render "notes/notes_with_form"

app/views/merge_requests/_show.html.haml

@@ -16,7 +16,7 @@
         Diff
 
 .merge_request_notes.voting_notes#notes{ class: (controller.action_name == 'show') ? "" : "hide" }
-  = render("notes/notes_with_form", tid: @merge_request.id, tt: "merge_request")
+  = render "notes/notes_with_form"
 .merge-request-diffs
   = render "merge_requests/show/diffs" if @diffs
 .status

app/views/merge_requests/show.js.haml

@@ -1,2 +1,2 @@
 :plain
-  $(".merge-request-notes").html("#{escape_javascript(render notes/notes_with_form", tid: @merge_request.id, tt: "merge_request")}");
+  $(".merge-request-notes").html("#{escape_javascript(render notes/notes_with_form")}");

app/views/notes/_common_form.html.haml

@@ -1,13 +1,16 @@
 .note-form-holder
   = form_for [@project, @note], remote: "true", multipart: true do |f|
+
+    = note_target_fields
+    = f.hidden_field :noteable_id
+    = f.hidden_field :noteable_type
+
     %h3.page_title Leave a comment
     -if @note.errors.any?
       .alert-message.block-message.error
         - @note.errors.full_messages.each do |msg|
           %div= msg
 
-    = f.hidden_field :noteable_id
-    = f.hidden_field :noteable_type
     = f.text_area :note, size: 255, class: 'js-note-text js-gfm-input'
     #preview-note.preview_note.hide
     .hint

app/views/notes/_create_diff_note.js.haml

@@ -1,14 +0,0 @@
-- if note.valid?
-  :plain
-    // hide and reset the form
-    var form = $("form[rel='#{note.discussion_id}']");
-    var row = form.closest("tr");
-
-    // 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
-      row.find(".notes").append("#{escape_javascript(render "notes/note", note: note)}");
-    }

app/views/notes/_create_discussion_note.js.haml

@@ -0,0 +1,13 @@
+- if note.valid?
+  :plain
+    // is this the first note of discussion?
+    var row = $("form[rel='#{note.discussion_id}']").closest("tr");
+    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])}");
+      // will be added again below
+      row.next().find(".note").remove();
+    }
+
+    // append new note to all discussions
+    $(".notes[rel='#{note.discussion_id}']").append("#{escape_javascript(render "notes/note", note: note)}");

app/views/notes/_diff_note_form.html.haml

@@ -1,26 +0,0 @@
-= 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|
-          %div= msg
-
-    = f.hidden_field :noteable_id
-    = f.hidden_field :noteable_type
-    = f.hidden_field :line_code
-    = f.text_area :note,  size: 255, class: 'js-note-text js-gfm-input'
-    .note_actions
-      .buttons
-        = 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
-          = label_tag :notify do
-            = check_box_tag :notify, 1, @note.noteable_type != "Commit"
-            %span Project team
-
-          - if @note.notify_only_author?(current_user)
-            = label_tag :notify_author do
-              = check_box_tag :notify_author, 1 , @note.noteable_type == "Commit"
-              %span Commit author

app/views/notes/_diff_notes_with_reply.html.haml

@@ -1,20 +1,11 @@
 - note = notes.first # example note
-%tr.notes_holder{ data: { :'discussion-id' => note.discussion_id } }
+%tr.notes_holder
   %td.notes_line{ colspan: 2 }
     %span.btn.disabled
       %i.icon-comment
       = notes.count
   %td.notes_content
-    %ul.notes
+    %ul.notes{ rel: note.discussion_id }
       = render notes
 
-    -# reply button
+    = render "notes/discussion_reply_button", note: note
-    = 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/_discussion.html.haml

@@ -30,11 +30,13 @@
           ago
   .discussion-body
     - if note.for_diff_line?
-      .diff_file.content
+      .content
-        = render "notes/discussion_diff", discussion_notes: discussion_notes, note: note
+        .diff_file= render "notes/discussion_diff", discussion_notes: discussion_notes, note: note
     - else
-      .notes.content
+      .content
+        .notes{ rel: discussion_notes.first.discussion_id }
           = render discussion_notes
+        = render "notes/discussion_reply_button", note: discussion_notes.first
 
     -# will be shown when the other one is hidden
     .discussion-hidden.content.hide

app/views/notes/_discussion_diff.html.haml

@@ -1,10 +1,11 @@
 - diff = note.diff
 .diff_file_header
-  %i.icon-file
   - if diff.deleted_file
-    %span{id: "#{diff.a_path}"}= diff.a_path
+    %span= diff.old_path
   - else
-    %span{id: "#{diff.b_path}"}= diff.b_path
+    %span= diff.new_path
+    - if diff.a_mode && diff.b_mode && diff.a_mode != diff.b_mode
+      %span.file-mode= "#{diff.a_mode} → #{diff.b_mode}"
   %br/
 .diff_file_content
   %table

app/views/notes/_discussion_note_form.html.haml

@@ -0,0 +1,28 @@
+= form_for [@project, @note], remote: true, html: { multipart: true, class: "new_note new_discussion_note js-discussion-note-form" } do |f|
+
+  = note_target_fields
+  = f.hidden_field :line_code
+  = f.hidden_field :noteable_id
+  = f.hidden_field :noteable_type
+
+  -if @note.errors.any?
+    .alert-message.block-message.error
+      - @note.errors.full_messages.each do |msg|
+        %div= msg
+
+  = f.text_area :note,  size: 255, class: 'js-note-text js-gfm-input'
+  .note_actions
+    .buttons
+      = f.submit 'Add Comment', class: "btn comment-btn js-comment-button"
+      %button.btn.js-close-discussion-note-form Cancel
+    .options
+      %h6.left Notify via email:
+      .labels
+        = label_tag :notify do
+          = check_box_tag :notify, 1, @note.noteable_type != "Commit"
+          %span Project team
+
+        - if @note.notify_only_author?(current_user)
+          = label_tag :notify_author do
+            = check_box_tag :notify_author, 1 , @note.noteable_type == "Commit"
+            %span Commit author

app/views/notes/_discussion_reply_button.html.haml

@@ -0,0 +1,9 @@
+= link_to "javascript:;",
+          class: "btn reply-btn js-discussion-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 reply" do
+  %i.icon-comment
+  Reply

app/views/notes/_notes_with_form.html.haml

@@ -1,13 +1,11 @@
 %ul#notes-list.notes
-%ul#new-notes-list.notes
-.notes-status
 
 - if can? current_user, :write_note, @project
   .note-forms.js-note-forms
     = render "notes/common_form"
-    = render "notes/diff_note_form"
+    = render "notes/discussion_note_form"
 
 :javascript
   $(function(){
-    NoteList.init("#{tid}", "#{tt}", "#{project_notes_path(@project)}");
+    NoteList.init("#{@target_id}", "#{@target_type}", "#{project_notes_path(@project)}");
   });

app/views/notes/_reversed_notes_with_form.html.haml

@@ -7,5 +7,5 @@
 
 :javascript
   $(function(){
-    NoteList.init("#{tid}", "#{tt}", "#{project_notes_path(@project)}");
+    NoteList.init("#{@target_id}", "#{@target_type}", "#{project_notes_path(@project)}");
   });

app/views/notes/create.js.haml

@@ -1,4 +1,4 @@
-- if @note.line_code
+- if note_for_main_target?(@note)
-  = render "create_diff_note", note: @note
-- else
   = render "create_common_note", note: @note
+- else
+  = render "create_discussion_note", note: @note

app/views/projects/wall.html.haml

@@ -1,2 +1,2 @@
 %div.wall_page
-  = render "notes/reversed_notes_with_form", tid: nil, tt: "wall"
+  = render "notes/reversed_notes_with_form"

app/views/snippets/show.html.haml

@@ -8,4 +8,4 @@
 
 %br
 %div= render 'blob'
-%div#notes= render "notes/notes_with_form", tid: @snippet.id, tt: "snippet"
+%div#notes= render "notes/notes_with_form"