diff options
author | Fatih Acet <acetfatih@gmail.com> | 2018-06-30 16:17:46 +0300 |
---|---|---|
committer | Fatih Acet <acetfatih@gmail.com> | 2018-06-30 16:17:46 +0300 |
commit | 923172c93310d2761ec1b64d181644a1a9e45ed6 (patch) | |
tree | 641b9407f3cebef6edfabb6aa473c30351cf1842 | |
parent | 3a3233a5b9e4be15bedd9004c8520475fd38f5c5 (diff) | |
parent | fb44cb3d904f487029c5d4ff955ed1a912908cc5 (diff) |
Merge branch 'tz-mr-diff-performance' into 'master'
Performance Improvements to Vue MR page
Closes #48492
See merge request gitlab-org/gitlab-ce!20246
-rw-r--r-- | app/assets/javascripts/diffs/components/app.vue | 2 | ||||
-rw-r--r-- | app/assets/javascripts/merge_request_tabs.js | 153 | ||||
-rw-r--r-- | app/assets/javascripts/notes/components/notes_app.vue | 2 | ||||
-rw-r--r-- | app/helpers/merge_requests_helper.rb | 2 | ||||
-rw-r--r-- | app/views/projects/merge_requests/creations/_new_submit.html.haml | 37 | ||||
-rw-r--r-- | spec/helpers/merge_requests_helper_spec.rb | 2 | ||||
-rw-r--r-- | spec/javascripts/helpers/init_vue_mr_page_helper.js | 12 | ||||
-rw-r--r-- | spec/javascripts/merge_request_spec.js | 23 | ||||
-rw-r--r-- | spec/javascripts/merge_request_tabs_spec.js | 1 |
9 files changed, 141 insertions, 93 deletions
diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index e0aecda54f6..eb0985e5603 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -145,7 +145,7 @@ export default { </script> <template> - <div v-if="shouldShow"> + <div v-show="shouldShow"> <div v-if="isLoading" class="loading" diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 7d6f1000ebe..53d7504de35 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -69,12 +69,23 @@ let { location } = window; export default class MergeRequestTabs { constructor({ action, setUrl, stubLocation } = {}) { - const mergeRequestTabs = document.querySelector('.js-tabs-affix'); + this.mergeRequestTabs = document.querySelector('.merge-request-tabs-container'); + this.mergeRequestTabsAll = + this.mergeRequestTabs && this.mergeRequestTabs.querySelectorAll + ? this.mergeRequestTabs.querySelectorAll('.merge-request-tabs li') + : null; + this.mergeRequestTabPanes = document.querySelector('#diff-notes-app'); + this.mergeRequestTabPanesAll = + this.mergeRequestTabPanes && this.mergeRequestTabPanes.querySelectorAll + ? this.mergeRequestTabPanes.querySelectorAll('.tab-pane') + : null; const navbar = document.querySelector('.navbar-gitlab'); const peek = document.getElementById('js-peek'); const paddingTop = 16; + this.commitsTab = document.querySelector('.tab-content .commits.tab-pane'); + this.currentTab = null; this.diffsLoaded = false; this.pipelinesLoaded = false; this.commitsLoaded = false; @@ -84,15 +95,15 @@ export default class MergeRequestTabs { this.setUrl = setUrl !== undefined ? setUrl : true; this.setCurrentAction = this.setCurrentAction.bind(this); this.tabShown = this.tabShown.bind(this); - this.showTab = this.showTab.bind(this); + this.clickTab = this.clickTab.bind(this); this.stickyTop = navbar ? navbar.offsetHeight - paddingTop : 0; if (peek) { this.stickyTop += peek.offsetHeight; } - if (mergeRequestTabs) { - this.stickyTop += mergeRequestTabs.offsetHeight; + if (this.mergeRequestTabs) { + this.stickyTop += this.mergeRequestTabs.offsetHeight; } if (stubLocation) { @@ -100,25 +111,22 @@ export default class MergeRequestTabs { } this.bindEvents(); - this.activateTab(action); + if ( + this.mergeRequestTabs && + this.mergeRequestTabs.querySelector(`a[data-action='${action}']`) && + this.mergeRequestTabs.querySelector(`a[data-action='${action}']`).click + ) + this.mergeRequestTabs.querySelector(`a[data-action='${action}']`).click(); this.initAffix(); } bindEvents() { - $(document) - .on('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown) - .on('click', '.js-show-tab', this.showTab); - - $('.merge-request-tabs a[data-toggle="tab"]').on('click', this.clickTab); + $('.merge-request-tabs a[data-toggle="tabvue"]').on('click', this.clickTab); } // Used in tests unbindEvents() { - $(document) - .off('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown) - .off('click', '.js-show-tab', this.showTab); - - $('.merge-request-tabs a[data-toggle="tab"]').off('click', this.clickTab); + $('.merge-request-tabs a[data-toggle="tabvue"]').off('click', this.clickTab); } destroyPipelinesView() { @@ -130,58 +138,87 @@ export default class MergeRequestTabs { } } - showTab(e) { - e.preventDefault(); - this.activateTab($(e.target).data('action')); - } - clickTab(e) { - if (e.currentTarget && isMetaClick(e)) { - const targetLink = e.currentTarget.getAttribute('href'); + if (e.currentTarget) { e.stopImmediatePropagation(); e.preventDefault(); - window.open(targetLink, '_blank'); + + const { action } = e.currentTarget.dataset; + + if (action) { + const href = e.currentTarget.getAttribute('href'); + this.tabShown(action, href); + } else if (isMetaClick(e)) { + const targetLink = e.currentTarget.getAttribute('href'); + window.open(targetLink, '_blank'); + } } } - tabShown(e) { - const $target = $(e.target); - const action = $target.data('action'); - - if (action === 'commits') { - this.loadCommits($target.attr('href')); - this.expandView(); - this.resetViewContainer(); - this.destroyPipelinesView(); - } else if (this.isDiffAction(action)) { - if (!isInVueNoteablePage()) { - this.loadDiff($target.attr('href')); - } - if (bp.getBreakpointSize() !== 'lg') { - this.shrinkView(); + tabShown(action, href) { + if (action !== this.currentTab && this.mergeRequestTabs) { + this.currentTab = action; + + if (this.mergeRequestTabPanesAll) { + this.mergeRequestTabPanesAll.forEach(el => { + const tabPane = el; + tabPane.style.display = 'none'; + }); } - if (this.diffViewType() === 'parallel') { - this.expandViewContainer(); + + if (this.mergeRequestTabsAll) { + this.mergeRequestTabsAll.forEach(el => { + el.classList.remove('active'); + }); } - this.destroyPipelinesView(); - this.commitsTab.classList.remove('active'); - } else if (action === 'pipelines') { - this.resetViewContainer(); - this.mountPipelinesView(); - } else { - if (bp.getBreakpointSize() !== 'xs') { + + const tabPane = this.mergeRequestTabPanes.querySelector(`#${action}`); + if (tabPane) tabPane.style.display = 'block'; + const tab = this.mergeRequestTabs.querySelector(`.${action}-tab`); + if (tab) tab.classList.add('active'); + + if (action === 'commits') { + this.loadCommits(href); + this.expandView(); + this.resetViewContainer(); + this.destroyPipelinesView(); + } else if (action === 'new') { this.expandView(); + this.resetViewContainer(); + this.destroyPipelinesView(); + } else if (this.isDiffAction(action)) { + if (!isInVueNoteablePage()) { + this.loadDiff(href); + } + if (bp.getBreakpointSize() !== 'lg') { + this.shrinkView(); + } + if (this.diffViewType() === 'parallel') { + this.expandViewContainer(); + } + this.destroyPipelinesView(); + this.commitsTab.classList.remove('active'); + } else if (action === 'pipelines') { + this.resetViewContainer(); + this.mountPipelinesView(); + } else { + this.mergeRequestTabPanes.querySelector('#notes').style.display = 'block'; + this.mergeRequestTabs.querySelector('.notes-tab').classList.add('active'); + + if (bp.getBreakpointSize() !== 'xs') { + this.expandView(); + } + this.resetViewContainer(); + this.destroyPipelinesView(); + + initDiscussionTab(); + } + if (this.setUrl) { + this.setCurrentAction(action); } - this.resetViewContainer(); - this.destroyPipelinesView(); - initDiscussionTab(); - } - if (this.setUrl) { - this.setCurrentAction(action); + this.eventHub.$emit('MergeRequestTabChange', this.getCurrentAction()); } - - this.eventHub.$emit('MergeRequestTabChange', this.getCurrentAction()); } scrollToElement(container) { @@ -194,12 +231,6 @@ export default class MergeRequestTabs { } } - // Activate a tab based on the current action - activateTab(action) { - // important note: the .tab('show') method triggers 'shown.bs.tab' event itself - $(`.merge-request-tabs a[data-action='${action}']`).tab('show'); - } - // Replaces the current Merge Request-specific action in the URL with a new one // // If the action is "notes", the URL is reset to the standard diff --git a/app/assets/javascripts/notes/components/notes_app.vue b/app/assets/javascripts/notes/components/notes_app.vue index 7853847fc37..a8995021699 100644 --- a/app/assets/javascripts/notes/components/notes_app.vue +++ b/app/assets/javascripts/notes/components/notes_app.vue @@ -175,7 +175,7 @@ export default { <template> <div - v-if="shouldShow" + v-show="shouldShow" id="notes" > <ul diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 82a7931c557..097be8a0643 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -108,7 +108,7 @@ module MergeRequestsHelper data_attrs = { action: tab.to_s, target: "##{tab}", - toggle: options.fetch(:force_link, false) ? '' : 'tab' + toggle: options.fetch(:force_link, false) ? '' : 'tabvue' } url = case tab diff --git a/app/views/projects/merge_requests/creations/_new_submit.html.haml b/app/views/projects/merge_requests/creations/_new_submit.html.haml index b2eacabc21a..f7a5d85500f 100644 --- a/app/views/projects/merge_requests/creations/_new_submit.html.haml +++ b/app/views/projects/merge_requests/creations/_new_submit.html.haml @@ -24,23 +24,28 @@ There are no commits yet. = custom_icon ('illustration_no_commits') - else - %ul.merge-request-tabs.nav.nav-tabs.nav-links.no-top.no-bottom - %li.commits-tab - = link_to url_for(safe_params), data: {target: 'div#commits', action: 'new', toggle: 'tab'} do - Commits - %span.badge.badge-pill= @commits.size - - if @pipelines.any? - %li.builds-tab - = link_to url_for(safe_params.merge(action: 'pipelines')), data: {target: 'div#pipelines', action: 'pipelines', toggle: 'tab'} do - Pipelines - %span.badge.badge-pill= @pipelines.size - %li.diffs-tab - = link_to url_for(safe_params.merge(action: 'diffs')), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do - Changes - %span.badge.badge-pill= @merge_request.diff_size + .merge-request-tabs-holder{ class: ("js-tabs-affix" unless ENV['RAILS_ENV'] == 'test') } + .merge-request-tabs-container + .scrolling-tabs-container.inner-page-scroll-tabs.is-smaller + .fade-left= icon('angle-left') + .fade-right= icon('angle-right') + %ul.merge-request-tabs.nav.nav-tabs.nav-links.no-top.no-bottom.js-tabs-affix + %li.commits-tab.new-tab + = link_to url_for(safe_params), data: {target: 'div#commits', action: 'new', toggle: 'tabvue'} do + Commits + %span.badge.badge-pill= @commits.size + - if @pipelines.any? + %li.builds-tab + = link_to url_for(safe_params.merge(action: 'pipelines')), data: {target: 'div#pipelines', action: 'pipelines', toggle: 'tabvue'} do + Pipelines + %span.badge.badge-pill= @pipelines.size + %li.diffs-tab + = link_to url_for(safe_params.merge(action: 'diffs')), data: {target: 'div#diffs', action: 'diffs', toggle: 'tabvue'} do + Changes + %span.badge.badge-pill= @merge_request.diff_size - .tab-content - #commits.commits.tab-pane.active + #diff-notes-app.tab-content + #new.commits.tab-pane.active = render "projects/merge_requests/commits" #diffs.diffs.tab-pane -# This tab is always loaded via AJAX diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb index 3008528e60c..885204062fe 100644 --- a/spec/helpers/merge_requests_helper_spec.rb +++ b/spec/helpers/merge_requests_helper_spec.rb @@ -54,7 +54,7 @@ describe MergeRequestsHelper do let(:options) { { force_link: true } } it 'removes the data-toggle attributes' do - is_expected.not_to match(/data-toggle="tab"/) + is_expected.not_to match(/data-toggle="tabvue"/) end end end diff --git a/spec/javascripts/helpers/init_vue_mr_page_helper.js b/spec/javascripts/helpers/init_vue_mr_page_helper.js index 05c6d587e9c..fc4288eb15b 100644 --- a/spec/javascripts/helpers/init_vue_mr_page_helper.js +++ b/spec/javascripts/helpers/init_vue_mr_page_helper.js @@ -5,12 +5,16 @@ import { userDataMock, notesDataMock, noteableDataMock } from '../notes/mock_dat import diffFileMockData from '../diffs/mock_data/diff_file'; export default function initVueMRPage() { + const mrTestEl = document.createElement('div'); + mrTestEl.className = 'js-merge-request-test'; + document.body.appendChild(mrTestEl); + const diffsAppEndpoint = '/diffs/app/endpoint'; const diffsAppProjectPath = 'testproject'; const mrEl = document.createElement('div'); mrEl.className = 'merge-request fixture-mr'; mrEl.setAttribute('data-mr-action', 'diffs'); - document.body.appendChild(mrEl); + mrTestEl.appendChild(mrEl); const mrDiscussionsEl = document.createElement('div'); mrDiscussionsEl.id = 'js-vue-mr-discussions'; @@ -18,18 +22,18 @@ export default function initVueMRPage() { mrDiscussionsEl.setAttribute('data-noteable-data', JSON.stringify(noteableDataMock)); mrDiscussionsEl.setAttribute('data-notes-data', JSON.stringify(notesDataMock)); mrDiscussionsEl.setAttribute('data-noteable-type', 'merge-request'); - document.body.appendChild(mrDiscussionsEl); + mrTestEl.appendChild(mrDiscussionsEl); const discussionCounterEl = document.createElement('div'); discussionCounterEl.id = 'js-vue-discussion-counter'; - document.body.appendChild(discussionCounterEl); + mrTestEl.appendChild(discussionCounterEl); const diffsAppEl = document.createElement('div'); diffsAppEl.id = 'js-diffs-app'; diffsAppEl.setAttribute('data-endpoint', diffsAppEndpoint); diffsAppEl.setAttribute('data-project-path', diffsAppProjectPath); diffsAppEl.setAttribute('data-current-user-data', JSON.stringify(userDataMock)); - document.body.appendChild(diffsAppEl); + mrTestEl.appendChild(diffsAppEl); const mock = new MockAdapter(axios); mock.onGet(diffsAppEndpoint).reply(200, { diff --git a/spec/javascripts/merge_request_spec.js b/spec/javascripts/merge_request_spec.js index 22eb0ad7143..7502f1fa2e1 100644 --- a/spec/javascripts/merge_request_spec.js +++ b/spec/javascripts/merge_request_spec.js @@ -19,9 +19,11 @@ import IssuablesHelper from '~/helpers/issuables_helper'; spyOn(axios, 'patch').and.callThrough(); mock = new MockAdapter(axios); - mock.onPatch(`${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`).reply(200, {}); + mock + .onPatch(`${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`) + .reply(200, {}); - return this.merge = new MergeRequest(); + return (this.merge = new MergeRequest()); }); afterEach(() => { @@ -32,17 +34,22 @@ import IssuablesHelper from '~/helpers/issuables_helper'; spyOn($, 'ajax').and.stub(); const changeEvent = document.createEvent('HTMLEvents'); changeEvent.initEvent('change', true, true); - $('input[type=checkbox]').attr('checked', true)[0].dispatchEvent(changeEvent); + $('input[type=checkbox]') + .attr('checked', true)[0] + .dispatchEvent(changeEvent); return expect($('.js-task-list-field').val()).toBe('- [x] Task List Item'); }); - it('submits an ajax request on tasklist:changed', (done) => { + it('submits an ajax request on tasklist:changed', done => { $('.js-task-list-field').trigger('tasklist:changed'); setTimeout(() => { - expect(axios.patch).toHaveBeenCalledWith(`${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`, { - merge_request: { description: '- [ ] Task List Item' }, - }); + expect(axios.patch).toHaveBeenCalledWith( + `${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`, + { + merge_request: { description: '- [ ] Task List Item' }, + }, + ); done(); }); }); @@ -119,4 +126,4 @@ import IssuablesHelper from '~/helpers/issuables_helper'; }); }); }); -}).call(window); +}.call(window)); diff --git a/spec/javascripts/merge_request_tabs_spec.js b/spec/javascripts/merge_request_tabs_spec.js index 08928e13985..7251ce19a90 100644 --- a/spec/javascripts/merge_request_tabs_spec.js +++ b/spec/javascripts/merge_request_tabs_spec.js @@ -40,6 +40,7 @@ describe('MergeRequestTabs', function() { this.class.unbindEvents(); this.class.destroyPipelinesView(); mrPageMock.restore(); + $('.js-merge-request-test').remove(); }); describe('opensInNewTab', function() { |