From a9441396daac861a381c50cef0766f929b1b26b6 Mon Sep 17 00:00:00 2001 From: Sam Bigelow Date: Mon, 25 Mar 2019 11:13:22 -0400 Subject: [PATCH] Scroll to diff file when clicking on file name - Add an ID to the diff content - handle clicking on file name in diffFileHeader when it is not a link to another page but rather a link to an element on the page --- .../diffs/components/diff_file.vue | 30 ++++---- .../diffs/components/diff_file_header.vue | 30 +++++++- .../javascripts/lib/utils/common_utils.js | 25 ++++--- ...ug-clicking-file-header-refreshes-page.yml | 6 ++ .../diffs/components/diff_file_header_spec.js | 71 ++++++++++++++++++- .../lib/utils/common_utils_spec.js | 33 +++++++++ 6 files changed, 171 insertions(+), 24 deletions(-) create mode 100644 changelogs/unreleased/57669-fix-bug-clicking-file-header-refreshes-page.yml diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index 0e779e1be9a..58a9605c181 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -170,21 +170,23 @@ export default {
{{ __('This source diff could not be displayed because it is too large.') }} diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue index a5125c3d077..1f08875be3b 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -11,6 +11,7 @@ import { __, s__, sprintf } from '~/locale'; import { diffViewerModes } from '~/ide/constants'; import EditButton from './edit_button.vue'; import DiffStats from './diff_stats.vue'; +import { scrollToElement } from '~/lib/utils/common_utils'; export default { components: { @@ -66,6 +67,9 @@ export default { hasExpandedDiscussions() { return this.diffHasExpandedDiscussions(this.diffFile); }, + diffContentIDSelector() { + return `#diff-content-${this.diffFile.file_hash}`; + }, icon() { if (this.diffFile.submodule) { return 'archive'; @@ -77,6 +81,11 @@ export default { if (this.diffFile.submodule) { return this.diffFile.submodule_tree_url || this.diffFile.submodule_link; } + + if (!this.discussionPath) { + return this.diffContentIDSelector; + } + return this.discussionPath; }, filePath() { @@ -152,6 +161,18 @@ export default { handleToggleDiscussions() { this.toggleFileDiscussions(this.diffFile); }, + handleFileNameClick(e) { + const isLinkToOtherPage = + this.diffFile.submodule_tree_url || this.diffFile.submodule_link || this.discussionPath; + + if (!isLinkToOtherPage) { + e.preventDefault(); + const selector = this.diffContentIDSelector; + + scrollToElement(document.querySelector(selector)); + window.location.hash = selector; + } + }, }, }; @@ -171,7 +192,14 @@ export default { class="diff-toggle-caret append-right-5" @click.stop="handleToggle" /> - + { const page = $('body').attr('data-page') || ''; @@ -193,16 +194,24 @@ export const isMetaKey = e => e.metaKey || e.ctrlKey || e.altKey || e.shiftKey; export const isMetaClick = e => e.metaKey || e.ctrlKey || e.which === 2; export const contentTop = () => { - const perfBar = $('#js-peek').height() || 0; - const mrTabsHeight = $('.merge-request-tabs').height() || 0; - const headerHeight = $('.navbar-gitlab').height() || 0; - const diffFilesChanged = $('.js-diff-files-changed').height() || 0; - const diffFileLargeEnoughScreen = - 'matchMedia' in window ? window.matchMedia('min-width: 768') : true; + const perfBar = $('#js-peek').outerHeight() || 0; + const mrTabsHeight = $('.merge-request-tabs').outerHeight() || 0; + const headerHeight = $('.navbar-gitlab').outerHeight() || 0; + const diffFilesChanged = $('.js-diff-files-changed').outerHeight() || 0; + const mdScreenOrBigger = ['lg', 'md'].includes(BreakpointInstance.getBreakpointSize()); const diffFileTitleBar = - (diffFileLargeEnoughScreen && $('.diff-file .file-title-flex-parent:visible').height()) || 0; + (mdScreenOrBigger && $('.diff-file .file-title-flex-parent:visible').outerHeight()) || 0; + const compareVersionsHeaderHeight = + (mdScreenOrBigger && $('.mr-version-controls').outerHeight()) || 0; - return perfBar + mrTabsHeight + headerHeight + diffFilesChanged + diffFileTitleBar; + return ( + perfBar + + mrTabsHeight + + headerHeight + + diffFilesChanged + + diffFileTitleBar + + compareVersionsHeaderHeight + ); }; export const scrollToElement = element => { diff --git a/changelogs/unreleased/57669-fix-bug-clicking-file-header-refreshes-page.yml b/changelogs/unreleased/57669-fix-bug-clicking-file-header-refreshes-page.yml new file mode 100644 index 00000000000..c6161870096 --- /dev/null +++ b/changelogs/unreleased/57669-fix-bug-clicking-file-header-refreshes-page.yml @@ -0,0 +1,6 @@ +--- +title: Scroll to diff file content when clicking on file header name and it is not + a link to other page +merge_request: !26422 +author: +type: fixed diff --git a/spec/javascripts/diffs/components/diff_file_header_spec.js b/spec/javascripts/diffs/components/diff_file_header_spec.js index 66c5b17b825..2d10b8383e2 100644 --- a/spec/javascripts/diffs/components/diff_file_header_spec.js +++ b/spec/javascripts/diffs/components/diff_file_header_spec.js @@ -3,7 +3,7 @@ import Vuex from 'vuex'; import diffsModule from '~/diffs/store/modules'; import notesModule from '~/notes/stores/modules'; import DiffFileHeader from '~/diffs/components/diff_file_header.vue'; -import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; +import mountComponent, { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; import diffDiscussionsMockData from '../mock_data/diff_discussions'; import { diffViewerModes } from '~/ide/constants'; @@ -252,6 +252,75 @@ describe('diff_file_header', () => { expect(vm.$emit).not.toHaveBeenCalled(); }); }); + + describe('handleFileNameClick', () => { + let e; + + beforeEach(() => { + e = { preventDefault: () => {} }; + spyOn(e, 'preventDefault'); + }); + + describe('when file name links to other page', () => { + it('does not call preventDefault if submodule tree url exists', () => { + vm = mountComponent(Component, { + ...props, + diffFile: { ...props.diffFile, submodule_tree_url: 'foobar.com' }, + }); + + vm.handleFileNameClick(e); + + expect(e.preventDefault).not.toHaveBeenCalled(); + }); + + it('does not call preventDefault if submodule_link exists', () => { + vm = mountComponent(Component, { + ...props, + diffFile: { ...props.diffFile, submodule_link: 'foobar.com' }, + }); + vm.handleFileNameClick(e); + + expect(e.preventDefault).not.toHaveBeenCalled(); + }); + + it('does not call preventDefault if discussionPath exists', () => { + vm = mountComponent(Component, { + ...props, + discussionPath: 'Foo bar', + }); + + vm.handleFileNameClick(e); + + expect(e.preventDefault).not.toHaveBeenCalled(); + }); + }); + + describe('scrolling to diff', () => { + let scrollToElement; + let el; + + beforeEach(() => { + el = document.createElement('div'); + spyOn(document, 'querySelector').and.returnValue(el); + scrollToElement = spyOnDependency(DiffFileHeader, 'scrollToElement'); + vm = mountComponent(Component, props); + + vm.handleFileNameClick(e); + }); + + it('calls scrollToElement with file content', () => { + expect(scrollToElement).toHaveBeenCalledWith(el); + }); + + it('element adds the content id to the window location', () => { + expect(window.location.hash).toContain(props.diffFile.file_hash); + }); + + it('calls preventDefault when button does not link to other page', () => { + expect(e.preventDefault).toHaveBeenCalled(); + }); + }); + }); }); describe('template', () => { diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js index 0bb43c94f6a..2084c36e484 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js +++ b/spec/javascripts/lib/utils/common_utils_spec.js @@ -2,6 +2,7 @@ import axios from '~/lib/utils/axios_utils'; import * as commonUtils from '~/lib/utils/common_utils'; import MockAdapter from 'axios-mock-adapter'; import { faviconDataUrl, overlayDataUrl, faviconWithOverlayDataUrl } from './mock_data'; +import BreakpointInstance from '~/breakpoints'; const PIXEL_TOLERANCE = 0.2; @@ -380,6 +381,38 @@ describe('common_utils', () => { }); }); + describe('contentTop', () => { + it('does not add height for fileTitle or compareVersionsHeader if screen is too small', () => { + spyOn(BreakpointInstance, 'getBreakpointSize').and.returnValue('sm'); + + setFixtures(` +
+ blah blah blah +
+
+ more blah blah blah +
+ `); + + expect(commonUtils.contentTop()).toBe(0); + }); + + it('adds height for fileTitle and compareVersionsHeader screen is large enough', () => { + spyOn(BreakpointInstance, 'getBreakpointSize').and.returnValue('lg'); + + setFixtures(` +
+ blah blah blah +
+
+ more blah blah blah +
+ `); + + expect(commonUtils.contentTop()).toBe(18); + }); + }); + describe('parseBoolean', () => { const { parseBoolean } = commonUtils;