From a7282b6563d8a00d954c12c93aae98e318a73565 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 24 Feb 2022 15:20:10 -0700 Subject: [PATCH 1/7] Extract page scrolling behavior to reusable function --- app/assets/javascripts/merge_request_tabs.js | 24 ++++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index ad0117844cd125..65ddf26783bb25 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -70,6 +70,16 @@ const FAST_DELAY_FOR_RERENDER = 75; // Store the `location` object, allowing for easier stubbing in tests let { location } = window; +function scrollToContainer(container) { + if (location.hash) { + const $el = $(`${container} ${location.hash}:not(.match)`); + + if ($el.length) { + scrollToElement($el[0]); + } + } +} + export default class MergeRequestTabs { constructor({ action, setUrl, stubLocation } = {}) { this.mergeRequestTabs = document.querySelector('.merge-request-tabs-container'); @@ -280,16 +290,6 @@ export default class MergeRequestTabs { this.eventHub.$emit('MergeRequestTabChange', action); } - scrollToContainerElement(container) { - if (location.hash) { - const $el = $(`${container} ${location.hash}:not(.match)`); - - if ($el.length) { - scrollToElement($el[0]); - } - } - } - // 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 @@ -365,7 +365,7 @@ export default class MergeRequestTabs { commitsDiv.innerHTML = data.html; localTimeAgo(commitsDiv.querySelectorAll('.js-timeago')); this.commitsLoaded = true; - this.scrollToContainerElement('#commits'); + scrollToContainer('#commits'); this.toggleLoading(false); @@ -445,7 +445,7 @@ export default class MergeRequestTabs { this.diffsLoaded = true; new Diff(); - this.scrollToContainerElement('#diffs'); + scrollToContainer('#diffs'); $('.diff-file').each((i, el) => { new BlobForkSuggestion({ -- GitLab From c3a897ad02ef65fc7995ab6d18995928f9a7c958 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 24 Feb 2022 15:26:28 -0700 Subject: [PATCH 2/7] Extract top sticky offset to a reusable function --- app/assets/javascripts/merge_request_tabs.js | 28 +++++++++----------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 65ddf26783bb25..78400a49550d5f 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -80,6 +80,18 @@ function scrollToContainer(container) { } } +function computeTopOffset(tabs) { + const navbar = document.querySelector('.navbar-gitlab'); + const peek = document.getElementById('js-peek'); + let stickyTop; + + stickyTop = navbar ? navbar.offsetHeight : 0; + stickyTop = peek ? stickyTop + peek.offsetHeight : stickyTop; + stickyTop = tabs ? stickyTop + tabs.offsetHeight : stickyTop; + + return stickyTop; +} + export default class MergeRequestTabs { constructor({ action, setUrl, stubLocation } = {}) { this.mergeRequestTabs = document.querySelector('.merge-request-tabs-container'); @@ -434,7 +446,7 @@ export default class MergeRequestTabs { .then(({ data }) => { const $container = $('#diffs'); $container.html(data.html); - initDiffStatsDropdown(this.stickyTop); + initDiffStatsDropdown(computeTopOffset(this.mergeRequestTabs)); localTimeAgo(document.querySelectorAll('#diffs .js-timeago')); syntaxHighlight($('#diffs .js-syntax-highlight')); @@ -529,18 +541,4 @@ export default class MergeRequestTabs { } }, 0); } - - get stickyTop() { - let stickyTop = this.navbar ? this.navbar.offsetHeight : 0; - - if (this.peek) { - stickyTop += this.peek.offsetHeight; - } - - if (this.mergeRequestTabs) { - stickyTop += this.mergeRequestTabs.offsetHeight; - } - - return stickyTop; - } } -- GitLab From bdfb19b42d2535276e849eab0d6efb51bc4914a7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 24 Feb 2022 15:58:12 -0700 Subject: [PATCH 3/7] Extract pipeline creation to reusable function --- app/assets/javascripts/merge_request_tabs.js | 71 +++++++++++--------- 1 file changed, 38 insertions(+), 33 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 78400a49550d5f..5e84199ef7beb9 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -92,6 +92,43 @@ function computeTopOffset(tabs) { return stickyTop; } +function mountPipelines() { + const pipelineTableViewEl = document.querySelector('#commit-pipeline-table-view'); + const { mrWidgetData } = gl; + const table = new Vue({ + components: { + CommitPipelinesTable: () => import('~/commit/pipelines/pipelines_table.vue'), + }, + provide: { + artifactsEndpoint: pipelineTableViewEl.dataset.artifactsEndpoint, + artifactsEndpointPlaceholder: pipelineTableViewEl.dataset.artifactsEndpointPlaceholder, + targetProjectFullPath: mrWidgetData?.target_project_full_path || '', + }, + render(createElement) { + return createElement('commit-pipelines-table', { + props: { + endpoint: pipelineTableViewEl.dataset.endpoint, + emptyStateSvgPath: pipelineTableViewEl.dataset.emptyStateSvgPath, + errorStateSvgPath: pipelineTableViewEl.dataset.errorStateSvgPath, + canCreatePipelineInTargetProject: Boolean( + mrWidgetData?.can_create_pipeline_in_target_project, + ), + sourceProjectFullPath: mrWidgetData?.source_project_full_path || '', + targetProjectFullPath: mrWidgetData?.target_project_full_path || '', + projectId: pipelineTableViewEl.dataset.projectId, + mergeRequestId: mrWidgetData ? mrWidgetData.iid : null, + }, + }); + }, + }).$mount(); + + // $mount(el) replaces the el with the new rendered component. We need it in order to mount + // it everytime this tab is clicked - https://vuejs.org/v2/api/#vm-mount + pipelineTableViewEl.appendChild(table.$el); + + return table; +} + export default class MergeRequestTabs { constructor({ action, setUrl, stubLocation } = {}) { this.mergeRequestTabs = document.querySelector('.merge-request-tabs-container'); @@ -393,39 +430,7 @@ export default class MergeRequestTabs { } mountPipelinesView() { - const pipelineTableViewEl = document.querySelector('#commit-pipeline-table-view'); - const { mrWidgetData } = gl; - - this.commitPipelinesTable = new Vue({ - components: { - CommitPipelinesTable: () => import('~/commit/pipelines/pipelines_table.vue'), - }, - provide: { - artifactsEndpoint: pipelineTableViewEl.dataset.artifactsEndpoint, - artifactsEndpointPlaceholder: pipelineTableViewEl.dataset.artifactsEndpointPlaceholder, - targetProjectFullPath: mrWidgetData?.target_project_full_path || '', - }, - render(createElement) { - return createElement('commit-pipelines-table', { - props: { - endpoint: pipelineTableViewEl.dataset.endpoint, - emptyStateSvgPath: pipelineTableViewEl.dataset.emptyStateSvgPath, - errorStateSvgPath: pipelineTableViewEl.dataset.errorStateSvgPath, - canCreatePipelineInTargetProject: Boolean( - mrWidgetData?.can_create_pipeline_in_target_project, - ), - sourceProjectFullPath: mrWidgetData?.source_project_full_path || '', - targetProjectFullPath: mrWidgetData?.target_project_full_path || '', - projectId: pipelineTableViewEl.dataset.projectId, - mergeRequestId: mrWidgetData ? mrWidgetData.iid : null, - }, - }); - }, - }).$mount(); - - // $mount(el) replaces the el with the new rendered component. We need it in order to mount - // it everytime this tab is clicked - https://vuejs.org/v2/api/#vm-mount - pipelineTableViewEl.appendChild(this.commitPipelinesTable.$el); + this.commitPipelinesTable = mountPipelines(); } // load the diff tab content from the backend -- GitLab From 1d7af24109484b68d456dcb5d938047b0954b6df Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 24 Feb 2022 16:09:01 -0700 Subject: [PATCH 4/7] Extract pipeline teardown to a reusable function --- app/assets/javascripts/merge_request_tabs.js | 27 ++++++++++---------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 5e84199ef7beb9..52b5a0ab44c7ec 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -129,6 +129,16 @@ function mountPipelines() { return table; } +function destroyPipelines(app) { + if (app && app.$destroy) { + app.$destroy(); + + document.querySelector('#commit-pipeline-table-view').innerHTML = ''; + } + + return null; +} + export default class MergeRequestTabs { constructor({ action, setUrl, stubLocation } = {}) { this.mergeRequestTabs = document.querySelector('.merge-request-tabs-container'); @@ -191,15 +201,6 @@ export default class MergeRequestTabs { $('.merge-request-tabs a[data-toggle="tabvue"]').off('click', this.clickTab); } - destroyPipelinesView() { - if (this.commitPipelinesTable) { - this.commitPipelinesTable.$destroy(); - this.commitPipelinesTable = null; - - document.querySelector('#commit-pipeline-table-view').innerHTML = ''; - } - } - storeScroll() { if (this.currentTab) { this.scrollPositions[this.currentTab] = document.documentElement.scrollTop; @@ -266,11 +267,11 @@ export default class MergeRequestTabs { this.loadCommits(href); this.expandView(); this.resetViewContainer(); - this.destroyPipelinesView(); + this.commitPipelinesTable = destroyPipelines(this.commitPipelinesTable); } else if (action === 'new') { this.expandView(); this.resetViewContainer(); - this.destroyPipelinesView(); + this.commitPipelinesTable = destroyPipelines(this.commitPipelinesTable); } else if (this.isDiffAction(action)) { if (!isInVueNoteablePage()) { /* @@ -287,7 +288,7 @@ export default class MergeRequestTabs { this.shrinkView(); } this.expandViewContainer(); - this.destroyPipelinesView(); + this.commitPipelinesTable = destroyPipelines(this.commitPipelinesTable); this.commitsTab.classList.remove('active'); } else if (action === 'pipelines') { this.resetViewContainer(); @@ -306,7 +307,7 @@ export default class MergeRequestTabs { this.expandView(); } this.resetViewContainer(); - this.destroyPipelinesView(); + this.commitPipelinesTable = destroyPipelines(this.commitPipelinesTable); } $('.detail-page-description').renderGFM(); -- GitLab From 4da3b55c5e894eb0a71789cf3aec066656935578 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 24 Feb 2022 16:25:36 -0700 Subject: [PATCH 5/7] Extract diff loading to a reusable function --- app/assets/javascripts/merge_request_tabs.js | 65 +++++++++++--------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 52b5a0ab44c7ec..13e2798681e0eb 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -139,6 +139,30 @@ function destroyPipelines(app) { return null; } +function loadDiffs({ url, sticky }) { + return axios.get(`${url}.json${location.search}`).then(({ data }) => { + const $container = $('#diffs'); + $container.html(data.html); + initDiffStatsDropdown(sticky); + + localTimeAgo(document.querySelectorAll('#diffs .js-timeago')); + syntaxHighlight($('#diffs .js-syntax-highlight')); + + new Diff(); + scrollToContainer('#diffs'); + + $('.diff-file').each((i, el) => { + new BlobForkSuggestion({ + openButtons: $(el).find('.js-edit-blob-link-fork-toggler'), + forkButtons: $(el).find('.js-fork-suggestion-button'), + cancelButtons: $(el).find('.js-cancel-fork-suggestion-button'), + suggestionSections: $(el).find('.js-file-fork-suggestion-section'), + actionTextPieces: $(el).find('.js-file-fork-suggestion-section-action'), + }).init(); + }); + }); +} + export default class MergeRequestTabs { constructor({ action, setUrl, stubLocation } = {}) { this.mergeRequestTabs = document.querySelector('.merge-request-tabs-container'); @@ -441,47 +465,28 @@ export default class MergeRequestTabs { return; } - // We extract pathname for the current Changes tab anchor href - // some pages like MergeRequestsController#new has query parameters on that anchor - const urlPathname = parseUrlPathname(source); - this.toggleLoading(true); - axios - .get(`${urlPathname}.json${location.search}`) - .then(({ data }) => { - const $container = $('#diffs'); - $container.html(data.html); - initDiffStatsDropdown(computeTopOffset(this.mergeRequestTabs)); - - localTimeAgo(document.querySelectorAll('#diffs .js-timeago')); - syntaxHighlight($('#diffs .js-syntax-highlight')); - + loadDiffs({ + // We extract pathname for the current Changes tab anchor href + // some pages like MergeRequestsController#new has query parameters on that anchor + url: parseUrlPathname(source), + sticky: computeTopOffset(this.mergeRequestTabs), + }) + .then(() => { if (this.isDiffAction(this.currentAction)) { this.expandViewContainer(); } - this.diffsLoaded = true; - - new Diff(); - scrollToContainer('#diffs'); - - $('.diff-file').each((i, el) => { - new BlobForkSuggestion({ - openButtons: $(el).find('.js-edit-blob-link-fork-toggler'), - forkButtons: $(el).find('.js-fork-suggestion-button'), - cancelButtons: $(el).find('.js-cancel-fork-suggestion-button'), - suggestionSections: $(el).find('.js-file-fork-suggestion-section'), - actionTextPieces: $(el).find('.js-file-fork-suggestion-section-action'), - }).init(); - }); - this.toggleLoading(false); + this.diffsLoaded = true; }) .catch(() => { - this.toggleLoading(false); createFlash({ message: __('An error occurred while fetching this tab.'), }); + }) + .finally(() => { + this.toggleLoading(false); }); } -- GitLab From 5887c86c1148acf12723da2b16c07e8466c86160 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 28 Feb 2022 14:57:31 -0700 Subject: [PATCH 6/7] Extract loading indicator toggle to a reusable function --- app/assets/javascripts/merge_request_tabs.js | 21 +++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 13e2798681e0eb..e510766744c335 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -163,6 +163,10 @@ function loadDiffs({ url, sticky }) { }); } +function toggleLoader(state) { + $('.mr-loading-status .loading').toggleClass('hide', !state); +} + export default class MergeRequestTabs { constructor({ action, setUrl, stubLocation } = {}) { this.mergeRequestTabs = document.querySelector('.merge-request-tabs-container'); @@ -430,7 +434,7 @@ export default class MergeRequestTabs { return; } - this.toggleLoading(true); + toggleLoader(true); axios .get(`${source}.json`) @@ -441,13 +445,13 @@ export default class MergeRequestTabs { this.commitsLoaded = true; scrollToContainer('#commits'); - this.toggleLoading(false); + toggleLoader(false); return import('./add_context_commits_modal'); }) .then((m) => m.default()) .catch(() => { - this.toggleLoading(false); + toggleLoader(false); createFlash({ message: __('An error occurred while fetching this tab.'), }); @@ -465,7 +469,7 @@ export default class MergeRequestTabs { return; } - this.toggleLoading(true); + toggleLoader(true); loadDiffs({ // We extract pathname for the current Changes tab anchor href @@ -486,17 +490,10 @@ export default class MergeRequestTabs { }); }) .finally(() => { - this.toggleLoading(false); + toggleLoader(false); }); } - // Show or hide the loading spinner - // - // status - Boolean, true to show, false to hide - toggleLoading(status) { - $('.mr-loading-status .loading').toggleClass('hide', !status); - } - diffViewType() { return $('.js-diff-view-buttons button.active').data('viewType'); } -- GitLab From 865f1c4bc73154d37d99c4a38003f2abefb1852d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 1 Mar 2022 02:36:16 -0700 Subject: [PATCH 7/7] Use optional chaining for clicking the tab --- app/assets/javascripts/merge_request_tabs.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index e510766744c335..5d28c95d8a2f73 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -204,13 +204,7 @@ export default class MergeRequestTabs { } this.bindEvents(); - 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.mergeRequestTabs?.querySelector(`a[data-action='${action}']`)?.click?.(); } bindEvents() { -- GitLab