From 76ae5af8ce9d88cb1da4f8b9836fe78b7c2ad30e Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Mon, 19 Sep 2016 18:02:52 -0500 Subject: [PATCH 1/2] ensure the 'fixed layout' preference is honored whenever possible see #22343 for issue description --- CHANGELOG | 1 + app/assets/javascripts/diff.js | 7 ++++ .../merge_conflict_data_provider.js.es6 | 12 ++++--- .../merge_conflict_resolver.js.es6 | 5 ++- app/assets/javascripts/merge_request.js | 9 ++--- app/assets/javascripts/merge_request_tabs.js | 34 ++++++++++++++++--- app/helpers/page_layout_helper.rb | 8 ++--- app/views/layouts/header/_default.html.haml | 2 +- app/views/profiles/preferences/update.js.erb | 4 +-- app/views/projects/diffs/_diffs.html.haml | 2 -- .../projects/merge_requests/_show.html.haml | 3 -- 11 files changed, 55 insertions(+), 32 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index b16302e0a03..1ff4bef86bd 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -27,6 +27,7 @@ v 8.12.0 (unreleased) - Use Search::GlobalService.new in the `GET /projects/search/:query` endpoint - Fix long comments in diffs messing with table width - Fix pagination on user snippets page + - Honor "fixed layout" preference in more places !6422 - Run CI builds with the permissions of users !5735 - Fix sorting of issues in API - Sort project variables by key. !6275 (Diego Souza) diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js index c8634b78f2b..8086c10ad6b 100644 --- a/app/assets/javascripts/diff.js +++ b/app/assets/javascripts/diff.js @@ -7,6 +7,9 @@ function Diff() { $('.files .diff-file').singleFileDiff(); this.filesCommentButton = $('.files .diff-file').filesCommentButton(); + if (this.diffViewType() === 'parallel') { + $('.content-wrapper .container-fluid').removeClass('container-limited'); + } $(document).off('click', '.js-unfold'); $(document).on('click', '.js-unfold', (function(_this) { return function(event) { @@ -52,6 +55,10 @@ })(this)); } + Diff.prototype.diffViewType = function() { + return $('.inline-parallel-buttons a.active').data('view-type'); + } + Diff.prototype.lineNumbers = function(line) { if (!line.children().length) { return [0, 0]; diff --git a/app/assets/javascripts/merge_conflict_data_provider.js.es6 b/app/assets/javascripts/merge_conflict_data_provider.js.es6 index cd92df8ddc5..f6e1409ee47 100644 --- a/app/assets/javascripts/merge_conflict_data_provider.js.es6 +++ b/app/assets/javascripts/merge_conflict_data_provider.js.es6 @@ -7,13 +7,16 @@ const ORIGIN_BUTTON_TITLE = 'Use theirs'; class MergeConflictDataProvider { getInitialData() { + // TODO: remove reliance on jQuery and DOM state introspection const diffViewType = $.cookie('diff_view'); + const fixedLayout = $('.content-wrapper .container-fluid').hasClass('container-limited'); return { isLoading : true, hasError : false, isParallel : diffViewType === 'parallel', diffViewType : diffViewType, + fixedLayout : fixedLayout, isSubmitting : false, conflictsData : {}, resolutionData : {} @@ -192,14 +195,15 @@ class MergeConflictDataProvider { updateViewType(newType) { const vi = this.vueInstance; - if (newType === vi.diffView || !(newType === 'parallel' || newType === 'inline')) { + if (newType === vi.diffViewType || !(newType === 'parallel' || newType === 'inline')) { return; } - vi.diffView = newType; - vi.isParallel = newType === 'parallel'; + vi.diffViewType = newType; + vi.isParallel = newType === 'parallel'; $.cookie('diff_view', newType); // TODO: Make sure that cookie path added. - $('.content-wrapper .container-fluid').toggleClass('container-limited'); + $('.content-wrapper .container-fluid') + .toggleClass('container-limited', !vi.isParallel && vi.fixedLayout); } diff --git a/app/assets/javascripts/merge_conflict_resolver.js.es6 b/app/assets/javascripts/merge_conflict_resolver.js.es6 index b56fd5aa658..7e756433bf5 100644 --- a/app/assets/javascripts/merge_conflict_resolver.js.es6 +++ b/app/assets/javascripts/merge_conflict_resolver.js.es6 @@ -60,9 +60,8 @@ class MergeConflictResolver { $('#conflicts .js-syntax-highlight').syntaxHighlight(); }); - if (this.vue.diffViewType === 'parallel') { - $('.content-wrapper .container-fluid').removeClass('container-limited'); - } + $('.content-wrapper .container-fluid') + .toggleClass('container-limited', !this.vue.isParallel && this.vue.fixedLayout); }) } diff --git a/app/assets/javascripts/merge_request.js b/app/assets/javascripts/merge_request.js index 05644b3d03c..02ff5a382e2 100644 --- a/app/assets/javascripts/merge_request.js +++ b/app/assets/javascripts/merge_request.js @@ -36,13 +36,10 @@ }; MergeRequest.prototype.initTabs = function() { - if (this.opts.action !== 'new') { - // `MergeRequests#new` has no tab-persisting or lazy-loading behavior - window.mrTabs = new MergeRequestTabs(this.opts); - } else { - // Show the first tab (Commits) - return $('.merge-request-tabs a[data-toggle="tab"]:first').tab('show'); + if (window.mrTabs) { + window.mrTabs.unbindEvents(); } + window.mrTabs = new MergeRequestTabs(this.opts); }; MergeRequest.prototype.showAllCommits = function() { diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 18bbfa7a459..bec11a198a1 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -56,6 +56,8 @@ MergeRequestTabs.prototype.commitsLoaded = false; + MergeRequestTabs.prototype.fixedLayoutPref = null; + function MergeRequestTabs(opts) { this.opts = opts != null ? opts : {}; this.opts.setUrl = this.opts.setUrl !== undefined ? this.opts.setUrl : true; @@ -70,7 +72,12 @@ MergeRequestTabs.prototype.bindEvents = function() { $(document).on('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown); - return $(document).on('click', '.js-show-tab', this.showTab); + $(document).on('click', '.js-show-tab', this.showTab); + }; + + MergeRequestTabs.prototype.unbindEvents = function() { + $(document).off('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown); + $(document).off('click', '.js-show-tab', this.showTab); }; MergeRequestTabs.prototype.showTab = function(event) { @@ -85,11 +92,15 @@ if (action === 'commits') { this.loadCommits($target.attr('href')); this.expandView(); + this.resetViewContainer(); } else if (action === 'diffs') { this.loadDiff($target.attr('href')); if ((typeof bp !== "undefined" && bp !== null) && bp.getBreakpointSize() !== 'lg') { this.shrinkView(); } + if (this.diffViewType() === 'parallel') { + this.expandViewContainer(); + } navBarHeight = $('.navbar-gitlab').outerHeight(); $.scrollTo(".merge-request-details .merge-request-tabs", { offset: -navBarHeight @@ -97,11 +108,14 @@ } else if (action === 'builds') { this.loadBuilds($target.attr('href')); this.expandView(); + this.resetViewContainer(); } else if (action === 'pipelines') { this.loadPipelines($target.attr('href')); this.expandView(); + this.resetViewContainer(); } else { this.expandView(); + this.resetViewContainer(); } if (this.opts.setUrl) { this.setCurrentAction(action); @@ -126,7 +140,7 @@ if (action === 'show') { action = 'notes'; } - return $(".merge-request-tabs a[data-action='" + action + "']").tab('show'); + $(".merge-request-tabs a[data-action='" + action + "']").tab('show').trigger('shown.bs.tab'); }; // Replaces the current Merge Request-specific action in the URL with a new one @@ -209,7 +223,7 @@ gl.utils.localTimeAgo($('.js-timeago', 'div#diffs')); $('#diffs .js-syntax-highlight').syntaxHighlight(); $('#diffs .diff-file').singleFileDiff(); - if (_this.diffViewType() === 'parallel') { + if (_this.diffViewType() === 'parallel' && _this.currentAction === 'diffs') { _this.expandViewContainer(); } _this.diffsLoaded = true; @@ -308,11 +322,21 @@ MergeRequestTabs.prototype.diffViewType = function() { return $('.inline-parallel-buttons a.active').data('view-type'); - // Returns diff view type }; MergeRequestTabs.prototype.expandViewContainer = function() { - return $('.container-fluid').removeClass('container-limited'); + var $wrapper = $('.content-wrapper .container-fluid'); + if (this.fixedLayoutPref === null) { + this.fixedLayoutPref = $wrapper.hasClass('container-limited'); + } + $wrapper.removeClass('container-limited'); + }; + + MergeRequestTabs.prototype.resetViewContainer = function() { + if (this.fixedLayoutPref !== null) { + $('.content-wrapper .container-fluid') + .toggleClass('container-limited', this.fixedLayoutPref); + } }; MergeRequestTabs.prototype.shrinkView = function() { diff --git a/app/helpers/page_layout_helper.rb b/app/helpers/page_layout_helper.rb index 22387d66451..7d4d049101a 100644 --- a/app/helpers/page_layout_helper.rb +++ b/app/helpers/page_layout_helper.rb @@ -92,12 +92,8 @@ module PageLayoutHelper end end - def fluid_layout(enabled = false) - if @fluid_layout.nil? - @fluid_layout = (current_user && current_user.layout == "fluid") || enabled - else - @fluid_layout - end + def fluid_layout + current_user && current_user.layout == "fluid" end def blank_container(enabled = false) diff --git a/app/views/layouts/header/_default.html.haml b/app/views/layouts/header/_default.html.haml index 94c53882623..237280872f1 100644 --- a/app/views/layouts/header/_default.html.haml +++ b/app/views/layouts/header/_default.html.haml @@ -1,5 +1,5 @@ %header.navbar.navbar-fixed-top.navbar-gitlab{ class: nav_header_class } - %div{ class: fluid_layout ? "container-fluid" : "container-fluid" } + %div{ class: "container-fluid" } .header-content %button.side-nav-toggle{ type: 'button', "aria-label" => "Toggle global navigation" } %span.sr-only Toggle navigation diff --git a/app/views/profiles/preferences/update.js.erb b/app/views/profiles/preferences/update.js.erb index 4433cab7782..8966dd3fd86 100644 --- a/app/views/profiles/preferences/update.js.erb +++ b/app/views/profiles/preferences/update.js.erb @@ -4,9 +4,9 @@ $('body').addClass('<%= user_application_theme %>') // Toggle container-fluid class if ('<%= current_user.layout %>' === 'fluid') { - $('.content-wrapper').find('.container-fluid').removeClass('container-limited') + $('.content-wrapper .container-fluid').removeClass('container-limited') } else { - $('.content-wrapper').find('.container-fluid').addClass('container-limited') + $('.content-wrapper .container-fluid').addClass('container-limited') } // Re-enable the "Save" button diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 62aff36aadd..576e7ef021a 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -1,7 +1,5 @@ - show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true) - diff_files = diffs.diff_files -- if diff_view == :parallel - - fluid_layout true .content-block.oneline-block.files-changed .inline-parallel-buttons diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index d03ff9ec7e8..9f34ca9ff4e 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -4,9 +4,6 @@ - content_for :page_specific_javascripts do = page_specific_javascript_tag('diff_notes/diff_notes_bundle.js') -- if diff_view == :parallel - - fluid_layout true - .merge-request{'data-url' => merge_request_path(@merge_request)} = render "projects/merge_requests/show/mr_title" From 62ba000e2d5e12985476f4b831366c2766f52f0e Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Tue, 20 Sep 2016 00:23:55 -0500 Subject: [PATCH 2/2] fix diff_view cookie path within merge conflict page --- app/assets/javascripts/merge_conflict_data_provider.js.es6 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_conflict_data_provider.js.es6 b/app/assets/javascripts/merge_conflict_data_provider.js.es6 index f6e1409ee47..13ee794ba38 100644 --- a/app/assets/javascripts/merge_conflict_data_provider.js.es6 +++ b/app/assets/javascripts/merge_conflict_data_provider.js.es6 @@ -201,7 +201,9 @@ class MergeConflictDataProvider { vi.diffViewType = newType; vi.isParallel = newType === 'parallel'; - $.cookie('diff_view', newType); // TODO: Make sure that cookie path added. + $.cookie('diff_view', newType, { + path: (gon && gon.relative_url_root) || '/' + }); $('.content-wrapper .container-fluid') .toggleClass('container-limited', !vi.isParallel && vi.fixedLayout); }