From 3fcdc0fe04120593166097792814e4cdbb4cbfb2 Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Mon, 18 Jul 2016 19:03:46 +0300 Subject: [PATCH 1/3] Unify diff option buttons. --- app/assets/javascripts/application.js.coffee | 6 ---- .../javascripts/merge_request_tabs.js.coffee | 21 ++++++++++++++ app/helpers/diff_helper.rb | 3 -- app/views/projects/diffs/_diffs.html.haml | 29 ++++++++++++------- app/views/projects/diffs/_file.html.haml | 5 ---- 5 files changed, 40 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/application.js.coffee b/app/assets/javascripts/application.js.coffee index eceff6d91d5e..46dc852e308f 100644 --- a/app/assets/javascripts/application.js.coffee +++ b/app/assets/javascripts/application.js.coffee @@ -206,12 +206,6 @@ $ -> $('.header-content .navbar-collapse').toggle() $('.navbar-toggle').toggleClass('active') - # Show/hide comments on diff - $body.on "click", ".js-toggle-diff-comments", (e) -> - $(@).toggleClass('active') - $(@).closest(".diff-file").find(".notes_holder").toggle() - e.preventDefault() - $document.off "click", '.js-confirm-danger' $document.on "click", '.js-confirm-danger', (e) -> e.preventDefault() diff --git a/app/assets/javascripts/merge_request_tabs.js.coffee b/app/assets/javascripts/merge_request_tabs.js.coffee index 86539e0d725a..fd8ce9c4bcf7 100644 --- a/app/assets/javascripts/merge_request_tabs.js.coffee +++ b/app/assets/javascripts/merge_request_tabs.js.coffee @@ -164,6 +164,7 @@ class @MergeRequestTabs @diffsLoaded = true @scrollToElement("#diffs") @highlighSelectedLine() + @bindDiffOptionsEvents() @filesCommentButton = $('.files .diff-file').filesCommentButton() $(document) @@ -174,6 +175,26 @@ class @MergeRequestTabs @highlighSelectedLine() @scrollToElement("#diffs") + + bindDiffOptionsEvents: -> + + $('.inline-parallel-buttons .js-diff-comments-button').on 'click', (e) -> + e.preventDefault() + + $el = $ this + state = $el.data 'state' + newState = 'hidden' + newLabel = 'Show all comments' + + if state is 'hidden' + newLabel = 'Hide all comments' + newState = 'visible' + + $("#diffs .notes_holder").toggle() + $el.text newLabel + $el.data 'state', newState + + highlighSelectedLine: -> $('.hll').removeClass 'hll' locationHash = window.location.hash diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 75b029365f93..52d1f09e82ca 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -153,9 +153,6 @@ def params_with_whitespace end def toggle_whitespace_link(url, options) - options[:class] ||= '' - options[:class] << ' btn btn-default' - link_to "#{hide_whitespace? ? 'Show' : 'Hide'} whitespace changes", url, class: options[:class] end end diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 8ae433b48235..0539fb4e6d16 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -5,16 +5,25 @@ - diff_files = safe_diff_files(diffs, diff_refs: diff_refs, repository: project.repository) .content-block.oneline-block.files-changed - .inline-parallel-buttons - - if !expand_all_diffs? && diff_files.any? { |diff_file| diff_file.collapsed? } - = link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: 'html')), class: 'btn btn-default' - - if show_whitespace_toggle - - if current_controller?(:commit) - = commit_diff_whitespace_link(@project, @commit, class: 'hidden-xs') - - elsif current_controller?(:merge_requests) - = diff_merge_request_whitespace_link(@project, @merge_request, class: 'hidden-xs') - - elsif current_controller?(:compare) - = diff_compare_whitespace_link(@project, params[:from], params[:to], class: 'hidden-xs') + .inline-parallel-buttons.dropdown + %button.btn.btn-default{ type: "button", data: { toggle: "dropdown" } } + Options + %span.caret + %ul.dropdown-menu + %li + %a.is-unselectable.js-diff-comments-button{ href: "#", data: { state: "visible" } } + Hide all comments + %li + - if show_whitespace_toggle + - if current_controller?(:commit) + = commit_diff_whitespace_link(@project, @commit, {}) + - elsif current_controller?(:merge_requests) + = diff_merge_request_whitespace_link(@project, @merge_request, {}) + - elsif current_controller?(:compare) + = diff_compare_whitespace_link(@project, params[:from], params[:to]) + %li + - if !expand_all_diffs? && diff_files.any? { |diff_file| diff_file.collapsed? } + = link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: 'html')), class: 'is-unselectable' .btn-group = inline_diff_btn = parallel_diff_btn diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index c306909fb1ae..9af0e7684204 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -4,11 +4,6 @@ - unless diff_file.submodule? .file-actions.hidden-xs - - if blob_text_viewable?(blob) - = link_to '#', class: 'js-toggle-diff-comments btn active has-tooltip btn-file-option', title: "Toggle comments for this file" do - = icon('comment') - \ - - if editable_diff?(diff_file) = edit_blob_link(@merge_request.source_project, @merge_request.source_branch, diff_file.new_path, -- GitLab From 0b3369f8bf18a5382c82e4099ce206b3e37858fe Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Tue, 19 Jul 2016 01:59:52 +0300 Subject: [PATCH 2/3] Added CHANGELOG. --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 017cf92160f4..a2982cdb7ea8 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -142,6 +142,7 @@ v 8.10.0 (unreleased) - 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 + - Unify diff options in merge requests page with a options dropdown - 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 - Fix last update timestamp on issues not preserved on gitlab.com and project imports -- GitLab From d997109482136bda30744f56ea2cd4aba2b45848 Mon Sep 17 00:00:00 2001 From: Fatih Acet Date: Tue, 19 Jul 2016 02:00:15 +0300 Subject: [PATCH 3/3] Spec fixes for removed UI elements. --- app/views/projects/diffs/_diffs.html.haml | 6 +++--- features/project/merge_requests.feature | 9 ++++----- features/steps/project/merge_requests.rb | 14 ++++++++------ spec/features/expand_collapse_diffs_spec.rb | 3 ++- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 0539fb4e6d16..a2092d01cda2 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -11,7 +11,7 @@ %span.caret %ul.dropdown-menu %li - %a.is-unselectable.js-diff-comments-button{ href: "#", data: { state: "visible" } } + %a.js-diff-comments-button{ href: "#", data: { state: "visible" } } Hide all comments %li - if show_whitespace_toggle @@ -20,10 +20,10 @@ - elsif current_controller?(:merge_requests) = diff_merge_request_whitespace_link(@project, @merge_request, {}) - elsif current_controller?(:compare) - = diff_compare_whitespace_link(@project, params[:from], params[:to]) + = diff_compare_whitespace_link(@project, params[:from], params[:to], {}) %li - if !expand_all_diffs? && diff_files.any? { |diff_file| diff_file.collapsed? } - = link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: 'html')), class: 'is-unselectable' + = link_to 'Expand all', url_for(params.merge(expand_all_diffs: 1, format: 'html')), {class: 'expand-all'} .btn-group = inline_diff_btn = parallel_diff_btn diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 21768c15c170..c1d4ec822ebd 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -194,7 +194,7 @@ Feature: Project Merge Requests And I visit merge request page "Bug NS-05" And I click on the Changes tab And I leave a comment like "Line is wrong" on line 39 of the third file - And I click link "Hide inline discussion" of the third file + And I click link "Hide inline discussion" button Then I should not see a comment like "Line is wrong here" in the third file @javascript @@ -212,9 +212,8 @@ Feature: Project Merge Requests And I click on the Changes tab And I leave a comment like "Line is correct" on line 12 of the second file And I leave a comment like "Line is wrong" on line 39 of the third file - And I click link "Hide inline discussion" of the third file + And I click link "Hide inline discussion" button Then I should not see a comment like "Line is wrong here" in the third file - And I should still see a comment like "Line is correct" in the second file @javascript Scenario: I show comments on a merge request diff with comments in multiple files @@ -223,8 +222,8 @@ Feature: Project Merge Requests And I click on the Changes tab And I leave a comment like "Line is correct" on line 12 of the second file And I leave a comment like "Line is wrong" on line 39 of the third file - And I click link "Hide inline discussion" of the third file - And I click link "Show inline discussion" of the third file + And I click link "Hide inline discussion" button + And I click link "Show inline discussion" button Then I should see a comment like "Line is wrong" in the third file And I should still see a comment like "Line is correct" in the second file diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index da848afd48eb..77602646d062 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -401,15 +401,17 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps end end - step 'I click link "Hide inline discussion" of the third file' do - page.within '.files [id^=diff]:nth-child(3)' do - find('.js-toggle-diff-comments').trigger('click') + step 'I click link "Hide inline discussion" button' do + page.within '#diffs' do + find('.inline-parallel-buttons [data-toggle=dropdown]').trigger('click') + find('.js-diff-comments-button').trigger('click') end end - step 'I click link "Show inline discussion" of the third file' do - page.within '.files [id^=diff]:nth-child(3)' do - find('.js-toggle-diff-comments').trigger('click') + step 'I click link "Show inline discussion" button' do + page.within '#diffs' do + find('.inline-parallel-buttons [data-toggle=dropdown]').trigger('click') + find('.js-diff-comments-button').trigger('click') end end diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb index 688f68d3cff3..853441426dbd 100644 --- a/spec/features/expand_collapse_diffs_spec.rb +++ b/spec/features/expand_collapse_diffs_spec.rb @@ -210,7 +210,8 @@ def file_container(filename) context 'expanding all diffs' do before do - click_link('Expand all') + find('.inline-parallel-buttons [data-toggle=dropdown]').trigger('click') + find('.inline-parallel-buttons .expand-all').trigger('click') wait_for_ajax execute_script('window.ajaxUris = []; $(document).ajaxSend(function(event, xhr, settings) { ajaxUris.push(settings.url) });') end -- GitLab