From fadb393a4a5e893e4dcaf9cf01a2a663892d3369 Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Fri, 15 Jul 2016 02:02:09 +0300 Subject: [PATCH 1/5] Revert "Refactored .prev and .next methods" This reverts commit ec37d9553274e3bca58e4de44b932cdc72a37325. --- app/assets/javascripts/files_comment_button.js.coffee | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/files_comment_button.js.coffee b/app/assets/javascripts/files_comment_button.js.coffee index db0bf7082a99..2b9dc6419d5b 100644 --- a/app/assets/javascripts/files_comment_button.js.coffee +++ b/app/assets/javascripts/files_comment_button.js.coffee @@ -7,7 +7,6 @@ class @FilesCommentButton UNFOLDABLE_LINE_CLASS = 'js-unfold' EMPTY_CELL_CLASS = 'empty-cell' OLD_LINE_CLASS = 'old_line' - NEW_CLASS = 'new' LINE_COLUMN_CLASSES = ".#{LINE_NUMBER_CLASS}, .line_content" TEXT_FILE_SELECTOR = '.text-file' DEBOUNCE_TIMEOUT_DURATION = 100 @@ -64,20 +63,17 @@ class @FilesCommentButton getLineContent: (hoveredElement) -> return hoveredElement if hoveredElement.hasClass LINE_CONTENT_CLASS - $(".#{LINE_CONTENT_CLASS + @diffTypeClass hoveredElement}", hoveredElement.parent()) + $(hoveredElement).next ".#{LINE_CONTENT_CLASS}" getButtonParent: (hoveredElement) -> if @VIEW_TYPE is 'inline' return hoveredElement if hoveredElement.hasClass OLD_LINE_CLASS - $(".#{OLD_LINE_CLASS}", hoveredElement.parent()) + hoveredElement.parent().find ".#{OLD_LINE_CLASS}" else return hoveredElement if hoveredElement.hasClass LINE_NUMBER_CLASS - $(".#{LINE_NUMBER_CLASS + @diffTypeClass hoveredElement}", hoveredElement.parent()) - - diffTypeClass: (hoveredElement) -> - if hoveredElement.hasClass(NEW_CLASS) then '.new' else '.old' + $(hoveredElement).prev ".#{LINE_NUMBER_CLASS}" isMovingToSameType: (e) -> newButtonParent = @getButtonParent $(e.toElement) -- GitLab From 901b54d9351bda2e6982321d99dba95b36870a8c Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Fri, 15 Jul 2016 02:15:32 +0300 Subject: [PATCH 2/5] Fix inline view for comment button selector. --- app/assets/javascripts/files_comment_button.js.coffee | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/files_comment_button.js.coffee b/app/assets/javascripts/files_comment_button.js.coffee index 2b9dc6419d5b..63d453fa411b 100644 --- a/app/assets/javascripts/files_comment_button.js.coffee +++ b/app/assets/javascripts/files_comment_button.js.coffee @@ -63,7 +63,10 @@ class @FilesCommentButton getLineContent: (hoveredElement) -> return hoveredElement if hoveredElement.hasClass LINE_CONTENT_CLASS - $(hoveredElement).next ".#{LINE_CONTENT_CLASS}" + if @VIEW_TYPE is 'inline' + return $(hoveredElement).closest(LINE_HOLDER_CLASS).find ".#{LINE_CONTENT_CLASS}" + else + return $(hoveredElement).next ".#{LINE_CONTENT_CLASS}" getButtonParent: (hoveredElement) -> if @VIEW_TYPE is 'inline' -- GitLab From 97180b84684ceb318e77936f1aef852c3b930b54 Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Fri, 15 Jul 2016 03:05:50 +0300 Subject: [PATCH 3/5] Off mouse listener from document for diff comment button. --- app/assets/javascripts/files_comment_button.js.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/files_comment_button.js.coffee b/app/assets/javascripts/files_comment_button.js.coffee index 63d453fa411b..5ab82c39fcd0 100644 --- a/app/assets/javascripts/files_comment_button.js.coffee +++ b/app/assets/javascripts/files_comment_button.js.coffee @@ -17,6 +17,8 @@ class @FilesCommentButton debounce = _.debounce @render, DEBOUNCE_TIMEOUT_DURATION $(document) + .off 'mouseover', LINE_COLUMN_CLASSES + .off 'mouseleave', LINE_COLUMN_CLASSES .on 'mouseover', LINE_COLUMN_CLASSES, debounce .on 'mouseleave', LINE_COLUMN_CLASSES, @destroy @@ -64,7 +66,7 @@ class @FilesCommentButton return hoveredElement if hoveredElement.hasClass LINE_CONTENT_CLASS if @VIEW_TYPE is 'inline' - return $(hoveredElement).closest(LINE_HOLDER_CLASS).find ".#{LINE_CONTENT_CLASS}" + return $(hoveredElement).closest(LINE_HOLDER_CLASS).find ".#{LINE_CONTENT_CLASS}" else return $(hoveredElement).next ".#{LINE_CONTENT_CLASS}" -- GitLab From d413463b32438e054be11cab37b21bde9cc73d8b Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Mon, 18 Jul 2016 20:11:55 +0300 Subject: [PATCH 4/5] Updated CHANGELOG. --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 6b24b2c5b327..745aba5bee96 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -90,6 +90,7 @@ v 8.10.0 (unreleased) - Optimistic locking for Issues and Merge Requests (Title and description overriding prevention) - Redesign Builds and Pipelines pages - Change status color and icon for running builds + - Fix commenting issue in side by side diff view for unchanged lines - Fix markdown rendering for: consecutive labels references, label references that begin with a digit or contains `.` v 8.9.6 -- GitLab From 122e191b9e039b0e9e2119f1ebacbbf82dd9016c Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Tue, 19 Jul 2016 07:23:23 +0100 Subject: [PATCH 5/5] Added diff comments feature test --- spec/features/merge_requests/diffs_spec.rb | 94 ++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb index c9a0059645d9..35f5bfe46bee 100644 --- a/spec/features/merge_requests/diffs_spec.rb +++ b/spec/features/merge_requests/diffs_spec.rb @@ -22,4 +22,98 @@ expect(page).to have_css('.diffs.tab-pane.active') end end + + context 'when hovering over the parallel view diff file' do + let(:comment_button_class) { '.add-diff-note' } + + before(:each) do + visit diffs_namespace_project_merge_request_path @project.namespace, @project, @merge_request + click_link 'Side-by-side' + @old_line_number = first '.diff-line-num.old_line:not(.empty-cell)' + @new_line_number = first '.diff-line-num.new_line:not(.empty-cell)' + @old_line = first '.line_content[data-line-type="old"]' + @new_line = first '.line_content[data-line-type="new"]' + end + + it 'shows a comment button on the old side when hovering over an old line number' do + @old_line_number.hover + expect(@old_line_number).to have_css comment_button_class + expect(@new_line_number).not_to have_css comment_button_class + end + + it 'shows a comment button on the old side when hovering over an old line' do + @old_line.hover + expect(@old_line_number).to have_css comment_button_class + expect(@new_line_number).not_to have_css comment_button_class + end + + it 'shows a comment button on the new side when hovering over a new line number' do + @new_line_number.hover + expect(@new_line_number).to have_css comment_button_class + expect(@old_line_number).not_to have_css comment_button_class + end + + it 'shows a comment button on the new side when hovering over a new line' do + @new_line.hover + expect(@new_line_number).to have_css comment_button_class + expect(@old_line_number).not_to have_css comment_button_class + end + end + + context 'when hovering over the inline view diff file' do + let(:comment_button_class) { '.add-diff-note' } + + before(:each) do + visit diffs_namespace_project_merge_request_path @project.namespace, @project, @merge_request + click_link 'Inline' + @old_line_number = first '.diff-line-num.old_line:not(.unfold)' + @new_line_number = first '.diff-line-num.new_line:not(.unfold)' + @new_line = first '.line_content:not(.match)' + end + + it 'shows a comment button on the old side when hovering over an old line number' do + @old_line_number.hover + expect(@old_line_number).to have_css comment_button_class + expect(@new_line_number).not_to have_css comment_button_class + end + + it 'shows a comment button on the new side when hovering over a new line number' do + @new_line_number.hover + expect(@old_line_number).to have_css comment_button_class + expect(@new_line_number).not_to have_css comment_button_class + end + + it 'shows a comment button on the new side when hovering over a new line' do + @new_line.hover + expect(@old_line_number).to have_css comment_button_class + expect(@new_line_number).not_to have_css comment_button_class + end + end + + context 'when clicking a comment button' do + let(:test_note_comment) { 'this is a test note!' } + let(:note_class) { '.new-note' } + + before(:each) do + visit diffs_namespace_project_merge_request_path @project.namespace, @project, @merge_request + click_link 'Inline' + first('.diff-line-num.old_line:not(.unfold)').hover + find('.add-diff-note').click + end + + it 'shows a note form' do + expect(page).to have_css note_class + end + + it 'can be submitted and viewed' do + fill_in 'note[note]', with: :test_note_comment + click_button 'Comment' + expect(page).to have_content :test_note_comment + end + + it 'can be closed' do + find('.note-form-actions .btn-cancel').click + expect(page).not_to have_css note_class + end + end end -- GitLab