Commit cfa9ea60 authored by Jacob Schatz's avatar Jacob Schatz

Merge branch 'fix-side-by-side-comment-widget' into 'master'

Fix side by side comment widget

Fixes comment button for empty lines in side by side diff view.

Fixes #19824 
Fixes #19957

See merge request !5262
parents b4717017 c11e4d15
...@@ -130,6 +130,7 @@ v 8.10.0 (unreleased) ...@@ -130,6 +130,7 @@ v 8.10.0 (unreleased)
- Optimistic locking for Issues and Merge Requests (Title and description overriding prevention) - Optimistic locking for Issues and Merge Requests (Title and description overriding prevention)
- Redesign Builds and Pipelines pages - Redesign Builds and Pipelines pages
- Change status color and icon for running builds - 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 `.` - Fix markdown rendering for: consecutive labels references, label references that begin with a digit or contains `.`
- Project export filename now includes the project and namespace path - Project export filename now includes the project and namespace path
- Fix last update timestamp on issues not preserved on gitlab.com and project imports - Fix last update timestamp on issues not preserved on gitlab.com and project imports
......
...@@ -7,7 +7,6 @@ class @FilesCommentButton ...@@ -7,7 +7,6 @@ class @FilesCommentButton
UNFOLDABLE_LINE_CLASS = 'js-unfold' UNFOLDABLE_LINE_CLASS = 'js-unfold'
EMPTY_CELL_CLASS = 'empty-cell' EMPTY_CELL_CLASS = 'empty-cell'
OLD_LINE_CLASS = 'old_line' OLD_LINE_CLASS = 'old_line'
NEW_CLASS = 'new'
LINE_COLUMN_CLASSES = ".#{LINE_NUMBER_CLASS}, .line_content" LINE_COLUMN_CLASSES = ".#{LINE_NUMBER_CLASS}, .line_content"
TEXT_FILE_SELECTOR = '.text-file' TEXT_FILE_SELECTOR = '.text-file'
DEBOUNCE_TIMEOUT_DURATION = 100 DEBOUNCE_TIMEOUT_DURATION = 100
...@@ -18,6 +17,8 @@ class @FilesCommentButton ...@@ -18,6 +17,8 @@ class @FilesCommentButton
debounce = _.debounce @render, DEBOUNCE_TIMEOUT_DURATION debounce = _.debounce @render, DEBOUNCE_TIMEOUT_DURATION
$(document) $(document)
.off 'mouseover', LINE_COLUMN_CLASSES
.off 'mouseleave', LINE_COLUMN_CLASSES
.on 'mouseover', LINE_COLUMN_CLASSES, debounce .on 'mouseover', LINE_COLUMN_CLASSES, debounce
.on 'mouseleave', LINE_COLUMN_CLASSES, @destroy .on 'mouseleave', LINE_COLUMN_CLASSES, @destroy
...@@ -64,20 +65,20 @@ class @FilesCommentButton ...@@ -64,20 +65,20 @@ class @FilesCommentButton
getLineContent: (hoveredElement) -> getLineContent: (hoveredElement) ->
return hoveredElement if hoveredElement.hasClass LINE_CONTENT_CLASS return hoveredElement if hoveredElement.hasClass LINE_CONTENT_CLASS
$(".#{LINE_CONTENT_CLASS + @diffTypeClass hoveredElement}", hoveredElement.parent()) if @VIEW_TYPE is 'inline'
return $(hoveredElement).closest(LINE_HOLDER_CLASS).find ".#{LINE_CONTENT_CLASS}"
else
return $(hoveredElement).next ".#{LINE_CONTENT_CLASS}"
getButtonParent: (hoveredElement) -> getButtonParent: (hoveredElement) ->
if @VIEW_TYPE is 'inline' if @VIEW_TYPE is 'inline'
return hoveredElement if hoveredElement.hasClass OLD_LINE_CLASS return hoveredElement if hoveredElement.hasClass OLD_LINE_CLASS
$(".#{OLD_LINE_CLASS}", hoveredElement.parent()) hoveredElement.parent().find ".#{OLD_LINE_CLASS}"
else else
return hoveredElement if hoveredElement.hasClass LINE_NUMBER_CLASS return hoveredElement if hoveredElement.hasClass LINE_NUMBER_CLASS
$(".#{LINE_NUMBER_CLASS + @diffTypeClass hoveredElement}", hoveredElement.parent()) $(hoveredElement).prev ".#{LINE_NUMBER_CLASS}"
diffTypeClass: (hoveredElement) ->
if hoveredElement.hasClass(NEW_CLASS) then '.new' else '.old'
isMovingToSameType: (e) -> isMovingToSameType: (e) ->
newButtonParent = @getButtonParent $(e.toElement) newButtonParent = @getButtonParent $(e.toElement)
......
...@@ -22,4 +22,98 @@ feature 'Diffs URL', js: true, feature: true do ...@@ -22,4 +22,98 @@ feature 'Diffs URL', js: true, feature: true do
expect(page).to have_css('.diffs.tab-pane.active') expect(page).to have_css('.diffs.tab-pane.active')
end end
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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment