Commit 90a6be19 authored by Sean McGivern's avatar Sean McGivern

Ensure only renderable text diffs are collapsed

Other diffs (those that are too large to render anyway, image diffs,
diffs suppressed by .gitattributes) should be rendered immediately.
parent b8d3016a
...@@ -2,13 +2,15 @@ class @SingleDiff ...@@ -2,13 +2,15 @@ class @SingleDiff
LOADING_HTML = '<i class="fa fa-spinner fa-spin"></i>' 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>' 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) -> constructor: (@file) ->
@content = $('.diff-content', @file) @content = $('.diff-content', @file)
@diffForPath = @content.data 'diff-for-path' @diffForPath = @content.find('[data-diff-for-path]').data 'diff-for-path'
@setOpenState() @setOpenState()
$('.file-title > a', @file).on 'click', @toggleDiff $('.file-title > a', @file).on 'click', @toggleDiff
@enableToggleOnContent()
setOpenState: -> setOpenState: ->
if @diffForPath if @diffForPath
...@@ -18,11 +20,15 @@ class @SingleDiff ...@@ -18,11 +20,15 @@ class @SingleDiff
@contentHTML = @content.html() @contentHTML = @content.html()
return return
enableToggleOnContent: ->
@content.find('.nothing-here-block.diff-collapsed').on 'click', @toggleDiff
toggleDiff: (e) => toggleDiff: (e) =>
e.preventDefault() e.preventDefault()
@isOpen = !@isOpen @isOpen = !@isOpen
if not @isOpen and not @hasError if not @isOpen and not @hasError
@content.empty() @content.html COLLAPSED_HTML
@enableToggleOnContent
return return
if @contentHTML if @contentHTML
@setContentHTML() @setContentHTML()
......
...@@ -189,3 +189,7 @@ span.idiff { ...@@ -189,3 +189,7 @@ span.idiff {
border-bottom-right-radius: 2px; border-bottom-right-radius: 2px;
} }
} }
.nothing-here-block.diff-collapsed {
cursor: pointer;
}
...@@ -15,6 +15,7 @@ module DiffHelper ...@@ -15,6 +15,7 @@ module DiffHelper
diff_commit = commit_for_diff(diff_file) diff_commit = commit_for_diff(diff_file)
blob = project.repository.blob_for_diff(diff_commit, diff_file) blob = project.repository.blob_for_diff(diff_commit, diff_file)
@expand_all = true
locals = { locals = {
diff_file: diff_file, diff_file: diff_file,
......
...@@ -9,7 +9,11 @@ ...@@ -9,7 +9,11 @@
- if !project.repository.diffable?(blob) - if !project.repository.diffable?(blob)
.nothing-here-block This diff was suppressed by a .gitattributes entry. .nothing-here-block This diff was suppressed by a .gitattributes entry.
- elsif diff_file.diff_lines.length > 0 - 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 = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob
- else - else
= render "projects/diffs/text_file", diff_file: diff_file = render "projects/diffs/text_file", diff_file: diff_file
......
...@@ -16,9 +16,4 @@ ...@@ -16,9 +16,4 @@
= view_file_btn(diff_commit.id, diff_file, project) = 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
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
...@@ -76,7 +76,7 @@ feature 'Prioritize labels', feature: true do ...@@ -76,7 +76,7 @@ feature 'Prioritize labels', feature: true do
expect(page.all('li').last).to have_content('bug') expect(page.all('li').last).to have_content('bug')
end end
visit current_url refresh
wait_for_ajax wait_for_ajax
page.within('.prioritized-labels') do page.within('.prioritized-labels') do
......
...@@ -27,6 +27,14 @@ module CapybaraHelpers ...@@ -27,6 +27,14 @@ module CapybaraHelpers
end end
end 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 end
RSpec.configure do |config| RSpec.configure do |config|
......
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