diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index f6a27c7ec6a..e5c233ef4fc 100644 --- a/app/assets/javascripts/notes.js +++ b/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(''); + 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(); }); - } - }); - } -} diff --git a/app/assets/stylesheets/sections/commits.scss b/app/assets/stylesheets/sections/commits.scss index a58d19ea179..b96e460c4f3 100644 --- a/app/assets/stylesheets/sections/commits.scss +++ b/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 **/ diff --git a/app/assets/stylesheets/sections/notes.scss b/app/assets/stylesheets/sections/notes.scss index e678c424f79..97739017c2c 100644 --- a/app/assets/stylesheets/sections/notes.scss +++ b/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 { - background-color: $white; - padding-top: 0; + 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,62 +206,74 @@ p.notify_controls span{ font-weight: 700; } -.line_notes_row, .per_line_form { font-family: "Helvetica Neue", Helvetica, Arial, sans-serif; } +/** + * 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; + background: url("comment_add.png") no-repeat left 0; + width:32px; + height:32px; -.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; + opacity: 0.0; + filter: alpha(opacity=0); - .buttons { - float: left; - width: 300px; + &:hover { + opacity: 1.0; + filter: alpha(opacity=100); } - .options { - .labels { - float: left; - padding-left: 10px; - label { - padding: 6px 0; - margin: 0; - width: 120px; - } - } + } + + &:hover > td { + background: $hover !important; + + .add-diff-note { + opacity: 1.0; + filter: alpha(opacity=100); } } } -td .line_note_link { - position: absolute; - margin-left:-70px; - margin-top:-10px; - z-index: 10; - background: url("comment_add.png") no-repeat left 0; - width: 32px; - height: 32px; +/** + * Note Forms + */ - opacity: 0.0; - filter: alpha(opacity=0); - - &:hover { - opacity: 1.0; - filter: alpha(opacity=100); - } +.comment-btn { + @extend .save-btn; } - -.diff_file_content tr.line_holder:hover > td { background: $hover !important; } -.diff_file_content tr.line_holder:hover > td .line_note_link { - opacity: 1.0; - filter: alpha(opacity=100); -} - .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; diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index ca22af0de83..4f8b7bb9da3 100644 --- a/app/controllers/notes_controller.rb +++ b/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) diff --git a/app/views/commit/show.html.haml b/app/views/commit/show.html.haml index 1157ae7adce..cef6b2f5bd5 100644 --- a/app/views/commit/show.html.haml +++ b/app/views/commit/show.html.haml @@ -1,12 +1,9 @@ = 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(' | W: ' + w + 'px | H: ' + h + 'px'); }, this)); - }); - }); diff --git a/app/views/merge_requests/_show.html.haml b/app/views/merge_requests/_show.html.haml index 9c5f4af26c3..90df81b4dc3 100644 --- a/app/views/merge_requests/_show.html.haml +++ b/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({ diff --git a/app/views/merge_requests/diffs.js.haml b/app/views/merge_requests/diffs.js.haml index 98539985324..c758cf69015 100644 --- a/app/views/merge_requests/diffs.js.haml +++ b/app/views/merge_requests/diffs.js.haml @@ -1,7 +1,3 @@ :plain $(".merge-request-diffs").html("#{escape_javascript(render(partial: "merge_requests/show/diffs"))}"); - $(function(){ - PerLineNotes.init(); - }); - diff --git a/app/views/notes/_common_form.html.haml b/app/views/notes/_common_form.html.haml index a1e87c87d20..cc7eaec307f 100644 --- a/app/views/notes/_common_form.html.haml +++ b/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: diff --git a/app/views/notes/_create_diff_note.js.haml b/app/views/notes/_create_diff_note.js.haml index a0c065ea4f7..81ade996ea4 100644 --- a/app/views/notes/_create_diff_note.js.haml +++ b/app/views/notes/_create_diff_note.js.haml @@ -1,18 +1,14 @@ - 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)}"); } diff --git a/app/views/notes/_diff_note_form.html.haml b/app/views/notes/_diff_note_form.html.haml index 9210be977f6..91128b91bd2 100644 --- a/app/views/notes/_diff_note_form.html.haml +++ b/app/views/notes/_diff_note_form.html.haml @@ -1,31 +1,26 @@ -%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 - -if @note.errors.any? - .alert-message.block-message.error - - @note.errors.full_messages.each do |msg| - %div= msg += 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: 'line-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" - .options - %h6.left Notify via email: - .labels - = label_tag :notify do - = check_box_tag :notify, 1, @note.noteable_type != "Commit" - %span Project team + = 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 + - 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 diff --git a/app/views/notes/_diff_note_link.html.haml b/app/views/notes/_diff_note_link.html.haml index d25577eac5a..56eeeb102eb 100644 --- a/app/views/notes/_diff_note_link.html.haml +++ b/app/views/notes/_diff_note_link.html.haml @@ -1,6 +1,9 @@ +- 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" diff --git a/app/views/notes/_diff_notes_with_reply.html.haml b/app/views/notes/_diff_notes_with_reply.html.haml index e3e5b291393..8b12cf11c75 100644 --- a/app/views/notes/_diff_notes_with_reply.html.haml +++ b/app/views/notes/_diff_notes_with_reply.html.haml @@ -1,4 +1,5 @@ -%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 diff --git a/app/views/notes/_notes_with_form.html.haml b/app/views/notes/_notes_with_form.html.haml index 713f8aac668..918b36c4fd8 100644 --- a/app/views/notes/_notes_with_form.html.haml +++ b/app/views/notes/_notes_with_form.html.haml @@ -3,7 +3,9 @@ .notes-status - if can? current_user, :write_note, @project - = render "notes/common_form" + .note-forms.js-note-forms + = render "notes/common_form" + = render "notes/diff_note_form" :javascript $(function(){ diff --git a/app/views/notes/create.js.haml b/app/views/notes/create.js.haml index 9921312e6eb..da7306d441c 100644 --- a/app/views/notes/create.js.haml +++ b/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"); diff --git a/app/views/notes/index.js.haml b/app/views/notes/index.js.haml index cf46af6523b..ae70dcad289 100644 --- a/app/views/notes/index.js.haml +++ b/app/views/notes/index.js.haml @@ -16,7 +16,3 @@ - if loading_more_notes? :plain NoteList.finishedLoadingMore(); - -- if @has_diff - :plain - PerLineNotes.init();