summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--CHANGELOG3
-rw-r--r--app/assets/javascripts/diff.js.coffee4
-rw-r--r--app/assets/javascripts/files_comment_button.js.coffee97
-rw-r--r--app/assets/javascripts/merge_request_tabs.js.coffee2
-rw-r--r--app/helpers/diff_helper.rb2
-rw-r--r--app/helpers/notes_helper.rb23
-rw-r--r--app/mailers/emails/projects.rb3
-rw-r--r--app/views/projects/diffs/_diffs.html.haml2
-rw-r--r--app/views/projects/diffs/_line.html.haml13
-rw-r--r--app/views/projects/diffs/_match_line_parallel.html.haml4
-rw-r--r--app/views/projects/diffs/_parallel_view.html.haml29
-rw-r--r--app/views/projects/diffs/_text_file.html.haml7
-rw-r--r--app/views/projects/show.html.haml2
-rw-r--r--features/project/commits/diff_comments.feature4
-rw-r--r--features/steps/shared/diff_note.rb23
-rw-r--r--spec/features/notes_on_merge_requests_spec.rb3
-rw-r--r--spec/helpers/diff_helper_spec.rb2
17 files changed, 158 insertions, 65 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 99c23223124..eccdcc988fa 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -33,6 +33,8 @@ v 8.10.0 (unreleased)
- API: Todos !3188 (Robert Schilling)
- API: Expose shared groups for projects and shared projects for groups !5050 (Robert Schilling)
- Add "Enabled Git access protocols" to Application Settings
+ - Diffs will create button/diff form on demand no on server side
+ - Reduce size of HTML used by diff comment forms
- Fix user creation with stronger minimum password requirements !4054 (nathan-pmt)
- Only show New Snippet button to users that can create snippets.
- PipelinesFinder uses git cache data
@@ -62,6 +64,7 @@ v 8.10.0 (unreleased)
- Allow '?', or '&' for label names
- Fix importer for GitHub Pull Requests when a branch was reused across Pull Requests
- Add date when user joined the team on the member page
+ - Fix 404 redirect after validation fails importing a GitLab project
- Fix 404 redirect after validation fails importing a GitLab project
- Added setting to set new users by default as external !4545 (Dravere)
- Add min value for project limit field on user's form !3622 (jastkand)
diff --git a/app/assets/javascripts/diff.js.coffee b/app/assets/javascripts/diff.js.coffee
index feb908c1abb..83516f97552 100644
--- a/app/assets/javascripts/diff.js.coffee
+++ b/app/assets/javascripts/diff.js.coffee
@@ -1,7 +1,7 @@
class @Diff
UNFOLD_COUNT = 20
constructor: ->
- $('.files .diff-file').singleFileDiff()
+ @filesCommentButton = $('.files .diff-file').filesCommentButton()
$(document).off('click', '.js-unfold')
$(document).on('click', '.js-unfold', (event) =>
@@ -38,7 +38,7 @@ class @Diff
# see https://gitlab.com/gitlab-org/gitlab-ce/issues/707
indent: 1
- $.get(link, params, (response) =>
+ $.get(link, params, (response) ->
target.parent().replaceWith(response)
)
)
diff --git a/app/assets/javascripts/files_comment_button.js.coffee b/app/assets/javascripts/files_comment_button.js.coffee
new file mode 100644
index 00000000000..db0bf7082a9
--- /dev/null
+++ b/app/assets/javascripts/files_comment_button.js.coffee
@@ -0,0 +1,97 @@
+class @FilesCommentButton
+ COMMENT_BUTTON_CLASS = '.add-diff-note'
+ COMMENT_BUTTON_TEMPLATE = _.template '<button name="button" type="submit" class="btn <%- COMMENT_BUTTON_CLASS %> js-add-diff-note-button" title="Add a comment to this line"><i class="fa fa-comment-o"></i></button>'
+ LINE_HOLDER_CLASS = '.line_holder'
+ LINE_NUMBER_CLASS = 'diff-line-num'
+ LINE_CONTENT_CLASS = 'line_content'
+ 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
+
+ constructor: (@filesContainerElement) ->
+ @VIEW_TYPE = $('input#view[type=hidden]').val()
+
+ debounce = _.debounce @render, DEBOUNCE_TIMEOUT_DURATION
+
+ $(document)
+ .on 'mouseover', LINE_COLUMN_CLASSES, debounce
+ .on 'mouseleave', LINE_COLUMN_CLASSES, @destroy
+
+ render: (e) =>
+ $currentTarget = $(e.currentTarget)
+ buttonParentElement = @getButtonParent $currentTarget
+ return unless @shouldRender e, buttonParentElement
+
+ textFileElement = @getTextFileElement $currentTarget
+ lineContentElement = @getLineContent $currentTarget
+
+ buttonParentElement.append @buildButton
+ noteableType: textFileElement.attr 'data-noteable-type'
+ noteableID: textFileElement.attr 'data-noteable-id'
+ commitID: textFileElement.attr 'data-commit-id'
+ noteType: lineContentElement.attr 'data-note-type'
+ position: lineContentElement.attr 'data-position'
+ lineType: lineContentElement.attr 'data-line-type'
+ discussionID: lineContentElement.attr 'data-discussion-id'
+ lineCode: lineContentElement.attr 'data-line-code'
+ return
+
+ destroy: (e) =>
+ return if @isMovingToSameType e
+ $(COMMENT_BUTTON_CLASS, @getButtonParent $(e.currentTarget)).remove()
+ return
+
+ buildButton: (buttonAttributes) ->
+ initializedButtonTemplate = COMMENT_BUTTON_TEMPLATE
+ COMMENT_BUTTON_CLASS: COMMENT_BUTTON_CLASS.substr 1
+ $(initializedButtonTemplate).attr
+ 'data-noteable-type': buttonAttributes.noteableType
+ 'data-noteable-id': buttonAttributes.noteableID
+ 'data-commit-id': buttonAttributes.commitID
+ 'data-note-type': buttonAttributes.noteType
+ 'data-line-code': buttonAttributes.lineCode
+ 'data-position': buttonAttributes.position
+ 'data-discussion-id': buttonAttributes.discussionID
+ 'data-line-type': buttonAttributes.lineType
+
+ getTextFileElement: (hoveredElement) ->
+ $(hoveredElement.closest TEXT_FILE_SELECTOR)
+
+ getLineContent: (hoveredElement) ->
+ return hoveredElement if hoveredElement.hasClass LINE_CONTENT_CLASS
+
+ $(".#{LINE_CONTENT_CLASS + @diffTypeClass hoveredElement}", hoveredElement.parent())
+
+ getButtonParent: (hoveredElement) ->
+ if @VIEW_TYPE is 'inline'
+ return hoveredElement if hoveredElement.hasClass OLD_LINE_CLASS
+
+ $(".#{OLD_LINE_CLASS}", hoveredElement.parent())
+ 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'
+
+ isMovingToSameType: (e) ->
+ newButtonParent = @getButtonParent $(e.toElement)
+ return false unless newButtonParent
+ newButtonParent.is @getButtonParent $(e.currentTarget)
+
+ shouldRender: (e, buttonParentElement) ->
+ (not buttonParentElement.hasClass(EMPTY_CELL_CLASS) and \
+ not buttonParentElement.hasClass(UNFOLDABLE_LINE_CLASS) and \
+ $(COMMENT_BUTTON_CLASS, buttonParentElement).length is 0)
+
+$.fn.filesCommentButton = ->
+ return unless this and @parent().data('can-create-note')?
+
+ @each ->
+ unless $.data this, 'filesCommentButton'
+ $.data this, 'filesCommentButton', new FilesCommentButton $(this)
diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee
index d55c4a34c07..86539e0d725 100644
--- a/app/assets/javascripts/merge_request_tabs.js.coffee
+++ b/app/assets/javascripts/merge_request_tabs.js.coffee
@@ -153,7 +153,6 @@ class @MergeRequestTabs
loadDiff: (source) ->
return if @diffsLoaded
-
@_get
url: "#{source}.json" + @_location.search
success: (data) =>
@@ -165,6 +164,7 @@ class @MergeRequestTabs
@diffsLoaded = true
@scrollToElement("#diffs")
@highlighSelectedLine()
+ @filesCommentButton = $('.files .diff-file').filesCommentButton()
$(document)
.off 'click', '.diff-line-num a'
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index e29f665baec..adab901700c 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -37,7 +37,7 @@ module DiffHelper
end
def unfold_bottom_class(bottom)
- bottom ? 'js-unfold-bottom' : ''
+ bottom ? 'js-unfold js-unfold-bottom' : ''
end
def unfold_class(unfold)
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index 2302e65c537..98143dcee9b 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -24,7 +24,15 @@ module NotesHelper
}.to_json
end
- def link_to_new_diff_note(line_code, position, line_type = nil)
+ def diff_view_data
+ return {} unless @comments_target
+
+ @comments_target.slice(:noteable_id, :noteable_type, :commit_id)
+ end
+
+ def diff_view_line_data(line_code, position, line_type)
+ return if @diff_notes_disabled
+
use_legacy_diff_note = @use_legacy_diff_notes
# If the controller doesn't force the use of legacy diff notes, we
# determine this on a line-by-line basis by seeing if there already exist
@@ -41,11 +49,8 @@ module NotesHelper
end
data = {
- noteable_type: @comments_target[:noteable_type],
- noteable_id: @comments_target[:noteable_id],
- commit_id: @comments_target[:commit_id],
- line_type: line_type,
- line_code: line_code
+ line_code: line_code,
+ line_type: line_type,
}
if use_legacy_diff_note
@@ -73,11 +78,7 @@ module NotesHelper
)
end
- button_tag(class: 'btn add-diff-note js-add-diff-note-button',
- data: data,
- title: 'Add a comment to this line') do
- icon('comment-o')
- end
+ data
end
def link_to_reply_discussion(note, line_type = nil)
diff --git a/app/mailers/emails/projects.rb b/app/mailers/emails/projects.rb
index 236b6ab00d8..e0af7081411 100644
--- a/app/mailers/emails/projects.rb
+++ b/app/mailers/emails/projects.rb
@@ -29,7 +29,8 @@ module Emails
# used in notify layout
@target_url = @message.target_url
@project = Project.find(project_id)
-
+ @diff_notes_disabled = true
+
add_project_headers
headers['X-GitLab-Author'] = @message.author_username
diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml
index 5db70bbb478..20aaab5accf 100644
--- a/app/views/projects/diffs/_diffs.html.haml
+++ b/app/views/projects/diffs/_diffs.html.haml
@@ -23,7 +23,7 @@
- if diff_files.overflow?
= render 'projects/diffs/warning', diff_files: diff_files
-.files
+.files{data: {can_create_note: (!@diff_notes_disabled && can?(current_user, :create_note, @project))}}
- diff_files.each_with_index do |diff_file, index|
- diff_commit = commit_for_diff(diff_file)
- blob = diff_file.blob(diff_commit)
diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml
index 22cad00240a..5a8a131d10c 100644
--- a/app/views/projects/diffs/_line.html.haml
+++ b/app/views/projects/diffs/_line.html.haml
@@ -13,18 +13,15 @@
%td.line_content.match= line.text
- else
%td.old_line.diff-line-num{ class: type, data: { linenumber: line.old_pos } }
- - link_text = type == "new" ? "&nbsp;".html_safe : line.old_pos
+ - link_text = type == "new" ? " " : line.old_pos
- if plain
= link_text
- else
- = link_to "", "##{line_code}", id: line_code, data: { linenumber: link_text }
-
- - if !plain && !@diff_notes_disabled && can?(current_user, :create_note, @project)
- = link_to_new_diff_note(line_code, position)
+ %a{href: "##{line_code}", data: { linenumber: link_text }}
%td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } }
- - link_text = type == "old" ? "&nbsp;".html_safe : line.new_pos
+ - link_text = type == "old" ? " " : line.new_pos
- if plain
= link_text
- else
- = link_to "", "##{line_code}", id: line_code, data: { linenumber: link_text }
- %td.line_content.noteable_line{ class: [type, line_code], data: { line_code: line_code, position: position.to_json } }= diff_line_content(line.text, type)
+ %a{href: "##{line_code}", data: { linenumber: link_text }}
+ %td.line_content.noteable_line{ class: type, data: (diff_view_line_data(line_code, position, type) unless plain) }= diff_line_content(line.text, type)
diff --git a/app/views/projects/diffs/_match_line_parallel.html.haml b/app/views/projects/diffs/_match_line_parallel.html.haml
index 0cd888876e0..b9c0d9dcdfd 100644
--- a/app/views/projects/diffs/_match_line_parallel.html.haml
+++ b/app/views/projects/diffs/_match_line_parallel.html.haml
@@ -1,4 +1,4 @@
-%td.old_line.diff-line-num
+%td.old_line.diff-line-num.empty-cell
%td.line_content.parallel.match= line
-%td.new_line.diff-line-num
+%td.new_line.diff-line-num.empty-cell
%td.line_content.parallel.match= line
diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml
index 51f207dce94..d208fcee10b 100644
--- a/app/views/projects/diffs/_parallel_view.html.haml
+++ b/app/views/projects/diffs/_parallel_view.html.haml
@@ -1,39 +1,34 @@
/ Side-by-side diff view
-%div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight
+%div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight{ data: diff_view_data }
%table
- diff_file.parallel_diff_lines.each do |line|
- left = line[:left]
- right = line[:right]
%tr.line_holder.parallel
- if left[:type] == 'match'
- = render "projects/diffs/match_line_parallel", { line: left[:text],
- line_old: left[:number], line_new: right[:number] }
+ = render "projects/diffs/match_line_parallel", { line: left[:text] }
- elsif left[:type] == 'nonewline'
- %td.old_line.diff-line-num
+ %td.old_line.diff-line-num.empty-cell
%td.line_content.parallel.match= left[:text]
- %td.new_line.diff-line-num
+ %td.new_line.diff-line-num.empty-cell
%td.line_content.parallel.match= left[:text]
- else
- %td.old_line.diff-line-num{id: left[:line_code], class: [left[:type], ('empty-cell' unless left[:number])]}
- = link_to raw(left[:number]), "##{left[:line_code]}", id: left[:line_code]
- - if !@diff_notes_disabled && can?(current_user, :create_note, @project)
- = link_to_new_diff_note(left[:line_code], left[:position], 'old')
- %td.line_content.parallel.noteable_line{class: [left[:type], left[:line_code], ('empty-cell' if left[:text].empty?)], data: { line_code: left[:line_code], position: left[:position].to_json }}= diff_line_content(left[:text])
+ %td.old_line.diff-line-num{id: left[:line_code], class: [left[:type], ('empty-cell' unless left[:number])], data: { linenumber: left[:number] }}
+ %a{href: "##{left[:line_code]}" }= raw(left[:number])
+ %td.line_content.parallel.noteable_line{class: [left[:type], ('empty-cell' if left[:text].empty?)], data: diff_view_line_data(left[:line_code], left[:position], 'old')}= diff_line_content(left[:text])
- if right[:type] == 'new'
- - new_line_class = 'new'
+ - new_line_type = 'new'
- new_line_code = right[:line_code]
- new_position = right[:position]
- else
- - new_line_class = nil
+ - new_line_type = nil
- new_line_code = left[:line_code]
- new_position = left[:position]
- %td.new_line.diff-line-num{id: new_line_code, class: [new_line_class, ('empty-cell' unless right[:number])], data: { linenumber: right[:number] }}
- = link_to raw(right[:number]), "##{new_line_code}", id: new_line_code
- - if !@diff_notes_disabled && can?(current_user, :create_note, @project)
- = link_to_new_diff_note(new_line_code, new_position, 'new')
- %td.line_content.parallel.noteable_line{class: [new_line_class, new_line_code, ('empty-cell' if right[:text].empty?)], data: { line_code: new_line_code, position: new_position.to_json }}= diff_line_content(right[:text])
+ %td.new_line.diff-line-num{id: new_line_code, class: [new_line_type, ('empty-cell' unless right[:number])], data: { linenumber: right[:number] }}
+ %a{href: "##{new_line_code}" }= raw(right[:number])
+ %td.line_content.parallel.noteable_line{class: [new_line_type, ('empty-cell' if right[:text].empty?)], data: diff_view_line_data(new_line_code, new_position, 'new')}= diff_line_content(right[:text])
- unless @diff_notes_disabled
- notes_left, notes_right = organize_comments(left, right)
diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml
index d61292c4bcb..196f8122db3 100644
--- a/app/views/projects/diffs/_text_file.html.haml
+++ b/app/views/projects/diffs/_text_file.html.haml
@@ -1,4 +1,9 @@
-%table.text-file.code.js-syntax-highlight
+- too_big = diff_file.diff_lines.count > Commit::DIFF_SAFE_LINES
+- if too_big
+ .suppressed-container
+ %a.show-suppressed-diff.js-show-suppressed-diff Changes suppressed. Click to show.
+
+%table.text-file.code.js-syntax-highlight{ data: diff_view_data, class: too_big ? 'hide' : '' }
- last_line = 0
- diff_file.highlighted_diff_lines.each do |line|
- last_line = line.new_pos
diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml
index 58d8e068754..dd1cf680cfa 100644
--- a/app/views/projects/show.html.haml
+++ b/app/views/projects/show.html.haml
@@ -82,4 +82,4 @@
Archived project! Repository is read-only
%div{class: "project-show-#{default_project_view}"}
- = render default_project_view
+ = render default_project_view \ No newline at end of file
diff --git a/features/project/commits/diff_comments.feature b/features/project/commits/diff_comments.feature
index 2bde4c8a99b..35687aac9ea 100644
--- a/features/project/commits/diff_comments.feature
+++ b/features/project/commits/diff_comments.feature
@@ -6,10 +6,6 @@ Feature: Project Commits Diff Comments
And I visit project commit page
@javascript
- Scenario: I can access add diff comment buttons
- Then I should see add a diff comment button
-
- @javascript
Scenario: I can comment on a commit diff
Given I leave a diff comment like "Typo, please fix"
Then I should see a diff comment saying "Typo, please fix"
diff --git a/features/steps/shared/diff_note.rb b/features/steps/shared/diff_note.rb
index 56ef44ec969..4df4e89f5b9 100644
--- a/features/steps/shared/diff_note.rb
+++ b/features/steps/shared/diff_note.rb
@@ -25,17 +25,16 @@ module SharedDiffNote
page.within("form[data-line-code='#{sample_commit.line_code}']") do
fill_in "note[note]", with: "Typo, please fix"
- find(".js-comment-button").trigger("click")
- sleep 0.05
+ find(".js-comment-button").click
end
end
end
step 'I leave a diff comment in a parallel view on the left side like "Old comment"' do
- click_parallel_diff_line(sample_commit.line_code, 'old')
- page.within("#{diff_file_selector} form[data-line-code='#{sample_commit.line_code}']") do
+ click_parallel_diff_line(sample_commit.del_line_code, 'old')
+ page.within("#{diff_file_selector} form[data-line-code='#{sample_commit.del_line_code}']") do
fill_in "note[note]", with: "Old comment"
- find(".js-comment-button").trigger("click")
+ find(".js-comment-button").click
end
end
@@ -43,7 +42,7 @@ module SharedDiffNote
click_parallel_diff_line(sample_commit.line_code, 'new')
page.within("#{diff_file_selector} form[data-line-code='#{sample_commit.line_code}']") do
fill_in "note[note]", with: "New comment"
- find(".js-comment-button").trigger("click")
+ find(".js-comment-button").click
end
end
@@ -165,10 +164,6 @@ module SharedDiffNote
end
end
- step 'I should see add a diff comment button' do
- expect(page).to have_css('.js-add-diff-note-button')
- end
-
step 'I should see an empty diff comment form' do
page.within(diff_file_selector) do
expect(page).to have_field("note[note]", with: "")
@@ -215,7 +210,7 @@ module SharedDiffNote
end
step 'I click side-by-side diff button' do
- find('#parallel-diff-btn').trigger('click')
+ find('#parallel-diff-btn').click
end
step 'I see side-by-side diff button' do
@@ -227,10 +222,12 @@ module SharedDiffNote
end
def click_diff_line(code)
- find("button[data-line-code='#{code}']").trigger('click')
+ find(".line_holder[id='#{code}'] td:nth-of-type(1)").trigger 'mouseover'
+ find(".line_holder[id='#{code}'] button").trigger 'click'
end
def click_parallel_diff_line(code, line_type)
- find("button[data-line-code='#{code}'][data-line-type='#{line_type}']").trigger('click')
+ find(".line_content.parallel.#{line_type}[data-line-code='#{code}']").trigger 'mouseover'
+ find(".line_holder.parallel button[data-line-code='#{code}']").trigger 'click'
end
end
diff --git a/spec/features/notes_on_merge_requests_spec.rb b/spec/features/notes_on_merge_requests_spec.rb
index 5174168713c..dd016a36d1e 100644
--- a/spec/features/notes_on_merge_requests_spec.rb
+++ b/spec/features/notes_on_merge_requests_spec.rb
@@ -231,6 +231,7 @@ describe 'Comments', feature: true do
end
def click_diff_line(data = line_code)
- execute_script("$('button[data-line-code=\"#{data}\"]').click()")
+ find(".line_holder[id='#{data}'] td.line_content").hover
+ find(".line_holder[id='#{data}'] button").trigger('click')
end
end
diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb
index 65ca8760958..4b134a48410 100644
--- a/spec/helpers/diff_helper_spec.rb
+++ b/spec/helpers/diff_helper_spec.rb
@@ -44,7 +44,7 @@ describe DiffHelper do
end
it 'should return js class when bottom lines should be unfolded' do
- expect(unfold_bottom_class(true)).to eq('js-unfold-bottom')
+ expect(unfold_bottom_class(true)).to include('js-unfold-bottom')
end
end
View Lorry Controller Status