diff options
author | Phil Hughes <me@iamphill.com> | 2018-09-20 11:21:17 +0300 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2018-10-02 19:03:30 +0300 |
commit | a9356ceed26cd445b89dec8185d973d4897bced5 (patch) | |
tree | 3e21155fdb2da6dc04bc8e1cc9c0449f5cb075bf | |
parent | 0d52b9448b9010bcf8d5f561a79b42a2e5339e27 (diff) |
Merge branch '50347-fix-scrolling-to-diff-note-after-incremental-rendering' into 'master'
Resolve "Link to file in Changes tab of MR no longer works for all files after incremental rendering improvement"
Closes #50347
See merge request gitlab-org/gitlab-ce!21727
5 files changed, 215 insertions, 40 deletions
diff --git a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue index 0e306f39a9f..62fa34e835a 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue @@ -1,5 +1,5 @@ <script> -import { mapGetters } from 'vuex'; +import { mapGetters, mapActions } from 'vuex'; import DiffTableCell from './diff_table_cell.vue'; import { NEW_LINE_TYPE, @@ -63,7 +63,11 @@ export default { this.linePositionLeft = LINE_POSITION_LEFT; this.linePositionRight = LINE_POSITION_RIGHT; }, + mounted() { + this.scrollToLineIfNeededInline(this.line); + }, methods: { + ...mapActions('diffs', ['scrollToLineIfNeededInline']), handleMouseMove(e) { // To show the comment icon on the gutter we need to know if we hover the line. // Current table structure doesn't allow us to do this with CSS in both of the diff view types diff --git a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue index fb68d191091..fcc3b3e9117 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue @@ -1,4 +1,5 @@ <script> +import { mapActions } from 'vuex'; import $ from 'jquery'; import DiffTableCell from './diff_table_cell.vue'; import { @@ -64,7 +65,11 @@ export default { this.oldLineType = OLD_LINE_TYPE; this.parallelDiffViewType = PARALLEL_DIFF_VIEW_TYPE; }, + mounted() { + this.scrollToLineIfNeededParallel(this.line); + }, methods: { + ...mapActions('diffs', ['scrollToLineIfNeededParallel']), handleMouseMove(e) { const isHover = e.type === 'mouseover'; const hoveringCell = e.target.closest('td'); diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 027df2ec841..e60bb9dd7e3 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -2,7 +2,7 @@ import Vue from 'vue'; import axios from '~/lib/utils/axios_utils'; import Cookies from 'js-cookie'; import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils'; -import { mergeUrlParams } from '~/lib/utils/url_utility'; +import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility'; import { getDiffPositionByLineCode } from './utils'; import * as types from './mutation_types'; import { @@ -120,6 +120,25 @@ export const loadMoreLines = ({ commit }, options) => { }); }; +export const scrollToLineIfNeededInline = (_, line) => { + const hash = getLocationHash(); + + if (hash && line.lineCode === hash) { + handleLocationHash(); + } +}; + +export const scrollToLineIfNeededParallel = (_, line) => { + const hash = getLocationHash(); + + if ( + hash && + ((line.left && line.left.lineCode === hash) || (line.right && line.right.lineCode === hash)) + ) { + handleLocationHash(); + } +}; + export const loadCollapsedDiff = ({ commit }, file) => axios.get(file.loadCollapsedDiffUrl).then(res => { commit(types.ADD_COLLAPSED_DIFFS, { diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index 0849d97bc1a..30925940807 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -56,7 +56,8 @@ export const rstrip = val => { return val; }; -export const updateTooltipTitle = ($tooltipEl, newTitle) => $tooltipEl.attr('title', newTitle).tooltip('_fixTitle'); +export const updateTooltipTitle = ($tooltipEl, newTitle) => + $tooltipEl.attr('title', newTitle).tooltip('_fixTitle'); export const disableButtonIfEmptyField = (fieldSelector, buttonSelector, eventName = 'input') => { const field = $(fieldSelector); @@ -86,6 +87,7 @@ export const handleLocationHash = () => { const fixedTabs = document.querySelector('.js-tabs-affix'); const fixedDiffStats = document.querySelector('.js-diff-files-changed'); const fixedNav = document.querySelector('.navbar-gitlab'); + const performanceBar = document.querySelector('#js-peek'); let adjustment = 0; if (fixedNav) adjustment -= fixedNav.offsetHeight; @@ -102,6 +104,10 @@ export const handleLocationHash = () => { adjustment -= fixedDiffStats.offsetHeight; } + if (performanceBar) { + adjustment -= performanceBar.offsetHeight; + } + window.scrollBy(0, adjustment); }; @@ -131,21 +137,20 @@ export const parseUrlPathname = url => { return parsedUrl.pathname.charAt(0) === '/' ? parsedUrl.pathname : `/${parsedUrl.pathname}`; }; -const splitPath = (path = '') => path - .replace(/^\?/, '') - .split('&'); +const splitPath = (path = '') => path.replace(/^\?/, '').split('&'); -export const urlParamsToArray = (path = '') => splitPath(path) - .filter(param => param.length > 0) - .map(param => { - const split = param.split('='); - return [decodeURI(split[0]), split[1]].join('='); - }); +export const urlParamsToArray = (path = '') => + splitPath(path) + .filter(param => param.length > 0) + .map(param => { + const split = param.split('='); + return [decodeURI(split[0]), split[1]].join('='); + }); export const getUrlParamsArray = () => urlParamsToArray(window.location.search); -export const urlParamsToObject = (path = '') => splitPath(path) - .reduce((dataParam, filterParam) => { +export const urlParamsToObject = (path = '') => + splitPath(path).reduce((dataParam, filterParam) => { if (filterParam === '') { return dataParam; } @@ -216,7 +221,7 @@ export const getParameterByName = (name, urlToParse) => { return decodeURIComponent(results[2].replace(/\+/g, ' ')); }; -const handleSelectedRange = (range) => { +const handleSelectedRange = range => { const container = range.commonAncestorContainer; // add context to fragment if needed if (container.tagName === 'OL') { @@ -453,7 +458,7 @@ export const backOff = (fn, timeout = 60000) => { export const createOverlayIcon = (iconPath, overlayPath) => { const faviconImage = document.createElement('img'); - return new Promise((resolve) => { + return new Promise(resolve => { faviconImage.onload = () => { const size = 32; @@ -464,13 +469,29 @@ export const createOverlayIcon = (iconPath, overlayPath) => { const context = canvas.getContext('2d'); context.clearRect(0, 0, size, size); context.drawImage( - faviconImage, 0, 0, faviconImage.width, faviconImage.height, 0, 0, size, size, + faviconImage, + 0, + 0, + faviconImage.width, + faviconImage.height, + 0, + 0, + size, + size, ); const overlayImage = document.createElement('img'); overlayImage.onload = () => { context.drawImage( - overlayImage, 0, 0, overlayImage.width, overlayImage.height, 0, 0, size, size, + overlayImage, + 0, + 0, + overlayImage.width, + overlayImage.height, + 0, + 0, + size, + size, ); const faviconWithOverlayUrl = canvas.toDataURL(); @@ -483,17 +504,21 @@ export const createOverlayIcon = (iconPath, overlayPath) => { }); }; -export const setFaviconOverlay = (overlayPath) => { +export const setFaviconOverlay = overlayPath => { const faviconEl = document.getElementById('favicon'); - if (!faviconEl) { return null; } + if (!faviconEl) { + return null; + } const iconPath = faviconEl.getAttribute('data-original-href'); - return createOverlayIcon(iconPath, overlayPath).then(faviconWithOverlayUrl => faviconEl.setAttribute('href', faviconWithOverlayUrl)); + return createOverlayIcon(iconPath, overlayPath).then(faviconWithOverlayUrl => + faviconEl.setAttribute('href', faviconWithOverlayUrl), + ); }; -export const setFavicon = (faviconPath) => { +export const setFavicon = faviconPath => { const faviconEl = document.getElementById('favicon'); if (faviconEl && faviconPath) { faviconEl.setAttribute('href', faviconPath); @@ -518,7 +543,7 @@ export const setCiStatusFavicon = pageUrl => } return resetFavicon(); }) - .catch((error) => { + .catch(error => { resetFavicon(); throw error; }); diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index cfb8f862598..76df3908b97 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -5,7 +5,23 @@ import { INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE, } from '~/diffs/constants'; -import * as actions from '~/diffs/store/actions'; +import actions, { + setBaseConfig, + fetchDiffFiles, + assignDiscussionsToDiff, + removeDiscussionsFromDiff, + startRenderDiffsQueue, + setInlineDiffViewType, + setParallelDiffViewType, + showCommentForm, + cancelCommentForm, + loadMoreLines, + scrollToLineIfNeededInline, + scrollToLineIfNeededParallel, + loadCollapsedDiff, + expandAllFiles, + toggleFileDiscussions, +} from '~/diffs/store/actions'; import * as types from '~/diffs/store/mutation_types'; import { reduceDiscussionsToLineCodes } from '~/notes/stores/utils'; import axios from '~/lib/utils/axios_utils'; @@ -37,7 +53,7 @@ describe('DiffsStoreActions', () => { const projectPath = '/root/project'; testAction( - actions.setBaseConfig, + setBaseConfig, { endpoint, projectPath }, { endpoint: '', projectPath: '' }, [{ type: types.SET_BASE_CONFIG, payload: { endpoint, projectPath } }], @@ -55,7 +71,7 @@ describe('DiffsStoreActions', () => { mock.onGet(endpoint).reply(200, res); testAction( - actions.fetchDiffFiles, + fetchDiffFiles, {}, { endpoint }, [ @@ -139,7 +155,7 @@ describe('DiffsStoreActions', () => { const discussions = reduceDiscussionsToLineCodes([singleDiscussion]); testAction( - actions.assignDiscussionsToDiff, + assignDiscussionsToDiff, discussions, state, [ @@ -207,7 +223,7 @@ describe('DiffsStoreActions', () => { }; testAction( - actions.removeDiscussionsFromDiff, + removeDiscussionsFromDiff, singleDiscussion, state, [ @@ -251,8 +267,7 @@ describe('DiffsStoreActions', () => { }); }; - actions - .startRenderDiffsQueue({ state, commit: pseudoCommit }) + startRenderDiffsQueue({ state, commit: pseudoCommit }) .then(() => { expect(state.diffFiles[0].renderIt).toBeTruthy(); expect(state.diffFiles[1].renderIt).toBeTruthy(); @@ -268,7 +283,7 @@ describe('DiffsStoreActions', () => { describe('setInlineDiffViewType', () => { it('should set diff view type to inline and also set the cookie properly', done => { testAction( - actions.setInlineDiffViewType, + setInlineDiffViewType, null, {}, [{ type: types.SET_DIFF_VIEW_TYPE, payload: INLINE_DIFF_VIEW_TYPE }], @@ -286,7 +301,7 @@ describe('DiffsStoreActions', () => { describe('setParallelDiffViewType', () => { it('should set diff view type to parallel and also set the cookie properly', done => { testAction( - actions.setParallelDiffViewType, + setParallelDiffViewType, null, {}, [{ type: types.SET_DIFF_VIEW_TYPE, payload: PARALLEL_DIFF_VIEW_TYPE }], @@ -306,7 +321,7 @@ describe('DiffsStoreActions', () => { const payload = { lineCode: 'lineCode' }; testAction( - actions.showCommentForm, + showCommentForm, payload, {}, [{ type: types.ADD_COMMENT_FORM_LINE, payload }], @@ -321,7 +336,7 @@ describe('DiffsStoreActions', () => { const payload = { lineCode: 'lineCode' }; testAction( - actions.cancelCommentForm, + cancelCommentForm, payload, {}, [{ type: types.REMOVE_COMMENT_FORM_LINE, payload }], @@ -343,7 +358,7 @@ describe('DiffsStoreActions', () => { mock.onGet(endpoint).reply(200, contextLines); testAction( - actions.loadMoreLines, + loadMoreLines, options, {}, [ @@ -369,7 +384,7 @@ describe('DiffsStoreActions', () => { mock.onGet(file.loadCollapsedDiffUrl).reply(200, data); testAction( - actions.loadCollapsedDiff, + loadCollapsedDiff, file, {}, [ @@ -390,7 +405,7 @@ describe('DiffsStoreActions', () => { describe('expandAllFiles', () => { it('should change the collapsed prop from the diffFiles', done => { testAction( - actions.expandAllFiles, + expandAllFiles, null, {}, [ @@ -414,7 +429,7 @@ describe('DiffsStoreActions', () => { const dispatch = jasmine.createSpy('dispatch'); - actions.toggleFileDiscussions({ getters, dispatch }); + toggleFileDiscussions({ getters, dispatch }); expect(dispatch).toHaveBeenCalledWith( 'collapseDiscussion', @@ -432,7 +447,7 @@ describe('DiffsStoreActions', () => { const dispatch = jasmine.createSpy(); - actions.toggleFileDiscussions({ getters, dispatch }); + toggleFileDiscussions({ getters, dispatch }); expect(dispatch).toHaveBeenCalledWith( 'expandDiscussion', @@ -450,7 +465,7 @@ describe('DiffsStoreActions', () => { const dispatch = jasmine.createSpy(); - actions.toggleFileDiscussions({ getters, dispatch }); + toggleFileDiscussions({ getters, dispatch }); expect(dispatch).toHaveBeenCalledWith( 'expandDiscussion', @@ -459,4 +474,111 @@ describe('DiffsStoreActions', () => { ); }); }); + + describe('scrollToLineIfNeededInline', () => { + const lineMock = { + lineCode: 'ABC_123', + }; + + it('should not call handleLocationHash when there is not hash', () => { + window.location.hash = ''; + + const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub(); + + scrollToLineIfNeededInline({}, lineMock); + + expect(handleLocationHashSpy).not.toHaveBeenCalled(); + }); + + it('should not call handleLocationHash when the hash does not match any line', () => { + window.location.hash = 'XYZ_456'; + + const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub(); + + scrollToLineIfNeededInline({}, lineMock); + + expect(handleLocationHashSpy).not.toHaveBeenCalled(); + }); + + it('should call handleLocationHash only when the hash matches a line', () => { + window.location.hash = 'ABC_123'; + + const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub(); + + scrollToLineIfNeededInline( + {}, + { + lineCode: 'ABC_456', + }, + ); + scrollToLineIfNeededInline({}, lineMock); + scrollToLineIfNeededInline( + {}, + { + lineCode: 'XYZ_456', + }, + ); + + expect(handleLocationHashSpy).toHaveBeenCalled(); + expect(handleLocationHashSpy).toHaveBeenCalledTimes(1); + }); + }); + + describe('scrollToLineIfNeededParallel', () => { + const lineMock = { + left: null, + right: { + lineCode: 'ABC_123', + }, + }; + + it('should not call handleLocationHash when there is not hash', () => { + window.location.hash = ''; + + const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub(); + + scrollToLineIfNeededParallel({}, lineMock); + + expect(handleLocationHashSpy).not.toHaveBeenCalled(); + }); + + it('should not call handleLocationHash when the hash does not match any line', () => { + window.location.hash = 'XYZ_456'; + + const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub(); + + scrollToLineIfNeededParallel({}, lineMock); + + expect(handleLocationHashSpy).not.toHaveBeenCalled(); + }); + + it('should call handleLocationHash only when the hash matches a line', () => { + window.location.hash = 'ABC_123'; + + const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub(); + + scrollToLineIfNeededParallel( + {}, + { + left: null, + right: { + lineCode: 'ABC_456', + }, + }, + ); + scrollToLineIfNeededParallel({}, lineMock); + scrollToLineIfNeededParallel( + {}, + { + left: null, + right: { + lineCode: 'XYZ_456', + }, + }, + ); + + expect(handleLocationHashSpy).toHaveBeenCalled(); + expect(handleLocationHashSpy).toHaveBeenCalledTimes(1); + }); + }); }); |