From 2bf5661c5ca4e85a25506a1b4a6e3297139f1681 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 22 Oct 2021 12:11:03 +0100 Subject: [PATCH] Fix jump to next discussion on diffs tab not working This fixes a bug on the diffs tab with the jump to next discussion button not working when the virtual scroller is enabled. Closes https://gitlab.com/gitlab-org/gitlab/-/issues/342849 --- .../javascripts/diffs/components/app.vue | 2 +- .../diffs/components/tree_list.vue | 2 +- app/assets/javascripts/diffs/index.js | 2 +- app/assets/javascripts/diffs/store/actions.js | 10 ++-- .../javascripts/lib/utils/common_utils.js | 8 ++-- .../notes/mixins/discussion_navigation.js | 15 +++++- spec/frontend/diffs/components/app_spec.js | 15 ++++-- .../diffs/components/tree_list_spec.js | 4 +- spec/frontend/diffs/store/actions_spec.js | 4 +- spec/frontend/lib/utils/common_utils_spec.js | 8 ++++ .../mixins/discussion_navigation_spec.js | 46 +++++++++++++++++++ 11 files changed, 97 insertions(+), 19 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index cf0d2814136e80..66d06a3a1b6096 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -570,7 +570,7 @@ export default { jumpToFile(step) { const targetIndex = this.currentDiffIndex + step; if (targetIndex >= 0 && targetIndex < this.diffFiles.length) { - this.scrollToFile(this.diffFiles[targetIndex].file_path); + this.scrollToFile({ path: this.diffFiles[targetIndex].file_path }); } }, setTreeDisplay() { diff --git a/app/assets/javascripts/diffs/components/tree_list.vue b/app/assets/javascripts/diffs/components/tree_list.vue index 41d885d3dc157d..85e4199d1c1a38 100644 --- a/app/assets/javascripts/diffs/components/tree_list.vue +++ b/app/assets/javascripts/diffs/components/tree_list.vue @@ -98,7 +98,7 @@ export default { :file-row-component="$options.DiffFileRow" :current-diff-file-id="currentDiffFileId" @toggleTreeOpen="toggleTreeOpen" - @clickFile="scrollToFile" + @clickFile="(path) => scrollToFile({ path })" />

diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js index 1b1ab59b2b40b6..260ebdf2141e1a 100644 --- a/app/assets/javascripts/diffs/index.js +++ b/app/assets/javascripts/diffs/index.js @@ -138,7 +138,7 @@ export default function initDiffsApp(store) { ...mapActions('diffs', ['toggleFileFinder', 'scrollToFile']), openFile(file) { window.mrTabs.tabShown('diffs'); - this.scrollToFile(file.path); + this.scrollToFile({ path: file.path }); }, }, render(createElement) { diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 5c94c6b803b741..9e828450acf0be 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -518,7 +518,7 @@ export const toggleActiveFileByHash = ({ commit }, hash) => { commit(types.VIEW_DIFF_FILE, hash); }; -export const scrollToFile = ({ state, commit, getters }, path) => { +export const scrollToFile = ({ state, commit, getters }, { path, setHash = true }) => { if (!state.treeEntries[path]) return; const { fileHash } = state.treeEntries[path]; @@ -528,9 +528,11 @@ export const scrollToFile = ({ state, commit, getters }, path) => { if (getters.isVirtualScrollingEnabled) { eventHub.$emit('scrollToFileHash', fileHash); - setTimeout(() => { - window.history.replaceState(null, null, `#${fileHash}`); - }); + if (setHash) { + setTimeout(() => { + window.history.replaceState(null, null, `#${fileHash}`); + }); + } } else { document.location.hash = fileHash; diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index 813fd3dbb1ecd0..ecf04ee487975c 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -220,16 +220,16 @@ export const scrollToElement = (element, options = {}) => { // In the previous implementation, jQuery naturally deferred this scrolling. // Unfortunately, we're quite coupled to this implementation detail now. defer(() => { - const { duration = 200, offset = 0 } = options; + const { duration = 200, offset = 0, behavior = duration ? 'smooth' : 'auto' } = options; const y = el.getBoundingClientRect().top + window.pageYOffset + offset - contentTop(); - window.scrollTo({ top: y, behavior: duration ? 'smooth' : 'auto' }); + window.scrollTo({ top: y, behavior }); }); } }; -export const scrollToElementWithContext = (element) => { +export const scrollToElementWithContext = (element, options) => { const offsetMultiplier = -0.1; - return scrollToElement(element, { offset: window.innerHeight * offsetMultiplier }); + return scrollToElement(element, { ...options, offset: window.innerHeight * offsetMultiplier }); }; /** diff --git a/app/assets/javascripts/notes/mixins/discussion_navigation.js b/app/assets/javascripts/notes/mixins/discussion_navigation.js index 96974c4fa2de90..b313d1acdf108e 100644 --- a/app/assets/javascripts/notes/mixins/discussion_navigation.js +++ b/app/assets/javascripts/notes/mixins/discussion_navigation.js @@ -2,6 +2,8 @@ import { mapGetters, mapActions, mapState } from 'vuex'; import { scrollToElementWithContext, scrollToElement } from '~/lib/utils/common_utils'; import eventHub from '../event_hub'; +const isDiffsVirtualScrollingEnabled = () => window.gon?.features?.diffsVirtualScrolling; + /** * @param {string} selector * @returns {boolean} @@ -11,7 +13,9 @@ function scrollTo(selector, { withoutContext = false } = {}) { const scrollFunction = withoutContext ? scrollToElement : scrollToElementWithContext; if (el) { - scrollFunction(el); + scrollFunction(el, { + behavior: isDiffsVirtualScrollingEnabled() ? 'auto' : 'smooth', + }); return true; } @@ -81,8 +85,15 @@ function handleDiscussionJump(self, fn, discussionId = self.currentDiscussionId) const discussion = self.getDiscussion(targetId); const discussionFilePath = discussion?.diff_file?.file_path; + if (isDiffsVirtualScrollingEnabled()) { + window.location.hash = ''; + } + if (discussionFilePath) { - self.scrollToFile(discussionFilePath); + self.scrollToFile({ + path: discussionFilePath, + setHash: !isDiffsVirtualScrollingEnabled(), + }); } self.$nextTick(() => { diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 9b63f84e617f06..d50ac0529d61b4 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -388,15 +388,24 @@ describe('diffs/components/app', () => { wrapper.vm.jumpToFile(+1); - expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual(['diffs/scrollToFile', '222.js']); + expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual([ + 'diffs/scrollToFile', + { path: '222.js' }, + ]); store.state.diffs.currentDiffFileId = '222'; wrapper.vm.jumpToFile(+1); - expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual(['diffs/scrollToFile', '333.js']); + expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual([ + 'diffs/scrollToFile', + { path: '333.js' }, + ]); store.state.diffs.currentDiffFileId = '333'; wrapper.vm.jumpToFile(-1); - expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual(['diffs/scrollToFile', '222.js']); + expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual([ + 'diffs/scrollToFile', + { path: '222.js' }, + ]); }); it('does not jump to previous file from the first one', async () => { diff --git a/spec/frontend/diffs/components/tree_list_spec.js b/spec/frontend/diffs/components/tree_list_spec.js index f316a9fdf01423..31044b0818c828 100644 --- a/spec/frontend/diffs/components/tree_list_spec.js +++ b/spec/frontend/diffs/components/tree_list_spec.js @@ -113,7 +113,9 @@ describe('Diffs tree list component', () => { wrapper.find('.file-row').trigger('click'); - expect(wrapper.vm.$store.dispatch).toHaveBeenCalledWith('diffs/scrollToFile', 'app/index.js'); + expect(wrapper.vm.$store.dispatch).toHaveBeenCalledWith('diffs/scrollToFile', { + path: 'app/index.js', + }); }); it('renders as file list when renderTreeList is false', () => { diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 85734e05aeb644..11ef3985abc251 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -890,7 +890,7 @@ describe('DiffsStoreActions', () => { }, }; - scrollToFile({ state, commit, getters }, 'path'); + scrollToFile({ state, commit, getters }, { path: 'path' }); expect(document.location.hash).toBe('#test'); }); @@ -904,7 +904,7 @@ describe('DiffsStoreActions', () => { }, }; - scrollToFile({ state, commit, getters }, 'path'); + scrollToFile({ state, commit, getters }, { path: 'path' }); expect(commit).toHaveBeenCalledWith(types.VIEW_DIFF_FILE, 'test'); }); diff --git a/spec/frontend/lib/utils/common_utils_spec.js b/spec/frontend/lib/utils/common_utils_spec.js index f5a74ee7f09d2c..94b1e1cbb15e07 100644 --- a/spec/frontend/lib/utils/common_utils_spec.js +++ b/spec/frontend/lib/utils/common_utils_spec.js @@ -279,6 +279,14 @@ describe('common_utils', () => { top: elementTopWithContext, }); }); + + it('passes through behaviour', () => { + commonUtils.scrollToElementWithContext(`#${id}`, { behavior: 'smooth' }); + expect(window.scrollTo).toHaveBeenCalledWith({ + behavior: 'smooth', + top: elementTopWithContext, + }); + }); }); }); diff --git a/spec/frontend/notes/mixins/discussion_navigation_spec.js b/spec/frontend/notes/mixins/discussion_navigation_spec.js index 6a6e47ffcc5c7e..70a5ff5184a91c 100644 --- a/spec/frontend/notes/mixins/discussion_navigation_spec.js +++ b/spec/frontend/notes/mixins/discussion_navigation_spec.js @@ -1,4 +1,5 @@ import { shallowMount, createLocalVue } from '@vue/test-utils'; +import { nextTick } from 'vue'; import Vuex from 'vuex'; import { setHTMLFixture } from 'helpers/fixtures'; import createEventHub from '~/helpers/event_hub_factory'; @@ -7,12 +8,15 @@ import eventHub from '~/notes/event_hub'; import discussionNavigation from '~/notes/mixins/discussion_navigation'; import notesModule from '~/notes/stores/modules'; +let scrollToFile; const discussion = (id, index) => ({ id, resolvable: index % 2 === 0, active: true, notes: [{}], diff_discussion: true, + position: { new_line: 1, old_line: 1 }, + diff_file: { file_path: 'test.js' }, }); const createDiscussions = () => [...'abcde'].map(discussion); const createComponent = () => ({ @@ -45,6 +49,7 @@ describe('Discussion navigation mixin', () => { jest.spyOn(utils, 'scrollToElement'); expandDiscussion = jest.fn(); + scrollToFile = jest.fn(); const { actions, ...notesRest } = notesModule(); store = new Vuex.Store({ modules: { @@ -52,6 +57,10 @@ describe('Discussion navigation mixin', () => { ...notesRest, actions: { ...actions, expandDiscussion }, }, + diffs: { + namespaced: true, + actions: { scrollToFile }, + }, }, }); store.state.notes.discussions = createDiscussions(); @@ -136,6 +145,7 @@ describe('Discussion navigation mixin', () => { it('scrolls to element', () => { expect(utils.scrollToElement).toHaveBeenCalledWith( findDiscussion('div.discussion', expected), + { behavior: 'smooth' }, ); }); }); @@ -163,6 +173,7 @@ describe('Discussion navigation mixin', () => { expect(utils.scrollToElementWithContext).toHaveBeenCalledWith( findDiscussion('ul.notes', expected), + { behavior: 'smooth' }, ); }); }); @@ -203,10 +214,45 @@ describe('Discussion navigation mixin', () => { it('scrolls to discussion', () => { expect(utils.scrollToElement).toHaveBeenCalledWith( findDiscussion('div.discussion', expected), + { behavior: 'smooth' }, ); }); }); }); }); + + describe.each` + diffsVirtualScrolling + ${false} + ${true} + `('virtual scrolling feature is $diffsVirtualScrolling', ({ diffsVirtualScrolling }) => { + beforeEach(async () => { + window.gon = { features: { diffsVirtualScrolling } }; + + jest.spyOn(store, 'dispatch'); + + store.state.notes.currentDiscussionId = 'a'; + window.location.hash = 'test'; + wrapper.vm.jumpToNextDiscussion(); + + await nextTick(); + }); + + afterEach(() => { + window.gon = {}; + window.location.hash = ''; + }); + + it('resets location hash if diffsVirtualScrolling flag is true', () => { + expect(window.location.hash).toBe(diffsVirtualScrolling ? '' : '#test'); + }); + + it(`calls scrollToFile with setHash as ${diffsVirtualScrolling ? 'false' : 'true'}`, () => { + expect(store.dispatch).toHaveBeenCalledWith('diffs/scrollToFile', { + path: 'test.js', + setHash: !diffsVirtualScrolling, + }); + }); + }); }); }); -- GitLab