diff options
| -rw-r--r-- | app/assets/javascripts/single_diff.js.coffee | 10 | ||||
| -rw-r--r-- | app/assets/stylesheets/framework/files.scss | 4 | ||||
| -rw-r--r-- | app/helpers/diff_helper.rb | 1 | ||||
| -rw-r--r-- | app/views/projects/diffs/_content.html.haml | 6 | ||||
| -rw-r--r-- | app/views/projects/diffs/_file.html.haml | 7 | ||||
| -rw-r--r-- | spec/features/merge_requests/expand_collapse_diffs_spec.rb | 138 | ||||
| -rw-r--r-- | spec/features/projects/labels/update_prioritization_spec.rb | 2 | ||||
| -rw-r--r-- | spec/support/capybara_helpers.rb | 8 |
8 files changed, 166 insertions, 10 deletions
diff --git a/app/assets/javascripts/single_diff.js.coffee b/app/assets/javascripts/single_diff.js.coffee index 4d1c28c082b..76db716317b 100644 --- a/app/assets/javascripts/single_diff.js.coffee +++ b/app/assets/javascripts/single_diff.js.coffee @@ -2,13 +2,15 @@ class @SingleDiff LOADING_HTML = '<i class="fa fa-spinner fa-spin"></i>' ERROR_HTML = '<div class="nothing-here-block"><i class="fa fa-warning"></i> Could not load diff</div>' + COLLAPSED_HTML = '<div class="nothing-here-block diff-collapsed">This diff is collapsed. Click to expand it.</div>' constructor: (@file) -> @content = $('.diff-content', @file) - @diffForPath = @content.data 'diff-for-path' + @diffForPath = @content.find('[data-diff-for-path]').data 'diff-for-path' @setOpenState() $('.file-title > a', @file).on 'click', @toggleDiff + @enableToggleOnContent() setOpenState: -> if @diffForPath @@ -18,11 +20,15 @@ class @SingleDiff @contentHTML = @content.html() return + enableToggleOnContent: -> + @content.find('.nothing-here-block.diff-collapsed').on 'click', @toggleDiff + toggleDiff: (e) => e.preventDefault() @isOpen = !@isOpen if not @isOpen and not @hasError - @content.empty() + @content.html COLLAPSED_HTML + @enableToggleOnContent return if @contentHTML @setContentHTML() diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index 71e4b50f2af..02480689f09 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -189,3 +189,7 @@ span.idiff { border-bottom-right-radius: 2px; } } + +.nothing-here-block.diff-collapsed { + cursor: pointer; +} diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 9a5920edfa2..93f12198a58 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -15,6 +15,7 @@ module DiffHelper diff_commit = commit_for_diff(diff_file) blob = project.repository.blob_for_diff(diff_commit, diff_file) + @expand_all = true locals = { diff_file: diff_file, diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml index 832aa0c5a14..9c6a17c0a8c 100644 --- a/app/views/projects/diffs/_content.html.haml +++ b/app/views/projects/diffs/_content.html.haml @@ -9,7 +9,11 @@ - if !project.repository.diffable?(blob) .nothing-here-block This diff was suppressed by a .gitattributes entry. - elsif diff_file.diff_lines.length > 0 - - if diff_view == 'parallel' + - if diff_file.collapsed_by_default? && !@expand_all + - url = url_for(params.merge(action: :diff_for_path, path: diff_file.file_path, format: nil)) + .nothing-here-block.diff-collapsed{data: { diff_for_path: url } } + This diff is collapsed. Click to expand it. + - elsif diff_view == 'parallel' = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob - else = render "projects/diffs/text_file", diff_file: diff_file diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index c83ed55efe1..c306909fb1a 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -16,9 +16,4 @@ = view_file_btn(diff_commit.id, diff_file, project) - - if diff_file.collapsed_by_default? - - url = url_for(params.merge(action: :diff_for_path, path: diff_file.file_path, format: nil)) - .diff-content.diff-wrap-lines{data: { diff_for_path: url }} - .nothing-here-block File hidden by default; content for this element available at #{url} - - else - = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, diff_refs: diff_refs, blob: blob, project: project + = render 'projects/diffs/content', diff_file: diff_file, diff_commit: diff_commit, diff_refs: diff_refs, blob: blob, project: project diff --git a/spec/features/merge_requests/expand_collapse_diffs_spec.rb b/spec/features/merge_requests/expand_collapse_diffs_spec.rb new file mode 100644 index 00000000000..173ea3720da --- /dev/null +++ b/spec/features/merge_requests/expand_collapse_diffs_spec.rb @@ -0,0 +1,138 @@ +require 'spec_helper' + +feature 'Expand and collapse diffs', js: true, feature: true do + include WaitForAjax + + before do + login_as :admin + merge_request = create(:merge_request, source_branch: 'expand-collapse-diffs', target_branch: 'master') + project = merge_request.source_project + + # Ensure that undiffable.md is in .gitattributes + project.repository.copy_gitattributes('expand-collapse-diffs') + visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request) + execute_script('window.ajaxUris = []; $(document).ajaxSend(function(event, xhr, settings) { ajaxUris.push(settings.url) });') + end + + def file_container(filename) + find("[data-blob-diff-path*='#{filename}']") + end + + # Use define_method instead of let (which is memoized) so that this just works across a + # reload. + # + ['small_diff.md', 'large_diff.md', 'undiffable.md', 'too_large.md', 'too_large_image.jpg'].each do |file| + define_method(file.split('.').first) { file_container(file) } + end + + context 'visiting an existing merge request' do + it 'shows small diffs immediately' do + expect(small_diff).to have_selector('.code') + expect(small_diff).not_to have_selector('.nothing-here-block') + end + + it 'collapses larges diffs by default' do + expect(large_diff).not_to have_selector('.code') + expect(large_diff).to have_selector('.nothing-here-block') + end + + it 'shows non-renderable diffs as such immediately, regardless of their size' do + expect(undiffable).not_to have_selector('.code') + expect(undiffable).to have_selector('.nothing-here-block') + expect(undiffable).to have_content('gitattributes') + end + + it 'does not allow diffs that are larger than the maximum size to be expanded' do + expect(too_large).not_to have_selector('.code') + expect(too_large).to have_selector('.nothing-here-block') + expect(too_large).to have_content('too large') + end + + it 'shows image diffs immediately, regardless of their size' do + expect(too_large_image).not_to have_selector('.nothing-here-block') + expect(too_large_image).to have_selector('.image') + end + + context 'expanding a large diff' do + before do + click_link('large_diff.md') + wait_for_ajax + end + + it 'makes a request to get the content' do + ajax_uris = evaluate_script('ajaxUris') + + expect(ajax_uris).not_to be_empty + expect(ajax_uris.first).to include('large_diff.md') + end + + it 'shows the diff content' do + expect(large_diff).to have_selector('.code') + expect(large_diff).not_to have_selector('.nothing-here-block') + end + + context 'adding a comment to the expanded diff' do + let(:comment_text) { 'A comment' } + + before do + large_diff.find('.line_holder', match: :prefer_exact).hover + large_diff.find('.add-diff-note').click + large_diff.find('.note-textarea').send_keys comment_text + large_diff.find_button('Comment').click + wait_for_ajax + end + + it 'adds the comment' do + expect(large_diff.find('.notes')).to have_content comment_text + end + + context 'reloading the page' do + before { refresh } + + it 'collapses the large diff by default' do + expect(large_diff).not_to have_selector('.code') + expect(large_diff).to have_selector('.nothing-here-block') + end + + context 'expanding the diff' do + before do + click_link('large_diff.md') + wait_for_ajax + end + + it 'shows the diff content' do + expect(large_diff).to have_selector('.code') + expect(large_diff).not_to have_selector('.nothing-here-block') + end + + it 'shows the diff comment' do + expect(large_diff.find('.notes')).to have_content comment_text + end + end + end + end + end + + context 'collapsing an expanded diff' do + before { click_link('small_diff.md') } + + it 'hides the diff content' do + expect(small_diff).not_to have_selector('.code') + expect(small_diff).to have_selector('.nothing-here-block') + end + + context 're-expanding the same diff' do + before { click_link('small_diff.md') } + + it 'shows the diff content' do + expect(small_diff).to have_selector('.code') + expect(small_diff).not_to have_selector('.nothing-here-block') + end + + it 'does not make a new HTTP request' do + expect(evaluate_script('ajaxUris')).to be_empty + end + end + end + end +end diff --git a/spec/features/projects/labels/update_prioritization_spec.rb b/spec/features/projects/labels/update_prioritization_spec.rb index 6a39c302f55..98ba93b4036 100644 --- a/spec/features/projects/labels/update_prioritization_spec.rb +++ b/spec/features/projects/labels/update_prioritization_spec.rb @@ -76,7 +76,7 @@ feature 'Prioritize labels', feature: true do expect(page.all('li').last).to have_content('bug') end - visit current_url + refresh wait_for_ajax page.within('.prioritized-labels') do diff --git a/spec/support/capybara_helpers.rb b/spec/support/capybara_helpers.rb index 9b5c3065eed..b57a3493aff 100644 --- a/spec/support/capybara_helpers.rb +++ b/spec/support/capybara_helpers.rb @@ -27,6 +27,14 @@ module CapybaraHelpers end end end + + # Refresh the page. Calling `visit current_url` doesn't seem to work consistently. + # + def refresh + url = current_url + visit 'about:blank' + visit url + end end RSpec.configure do |config| |