diff options
| -rw-r--r-- | app/assets/javascripts/breakpoints.js | 3 | ||||
| -rw-r--r-- | app/assets/javascripts/diffs/components/app.vue | 15 | ||||
| -rw-r--r-- | app/assets/javascripts/diffs/components/compare_versions.vue | 18 | ||||
| -rw-r--r-- | app/assets/javascripts/diffs/components/diff_file_header.vue | 137 | ||||
| -rw-r--r-- | app/assets/javascripts/diffs/constants.js | 3 | ||||
| -rw-r--r-- | app/assets/javascripts/lib/utils/common_utils.js | 9 | ||||
| -rw-r--r-- | app/assets/stylesheets/framework/files.scss | 4 | ||||
| -rw-r--r-- | app/assets/stylesheets/pages/diff.scss | 8 | ||||
| -rw-r--r-- | app/assets/stylesheets/pages/merge_requests.scss | 16 | ||||
| -rw-r--r-- | changelogs/unreleased/57364-improve-diff-nav-header.yml | 5 | ||||
| -rw-r--r-- | spec/javascripts/diffs/components/app_spec.js | 18 | ||||
| -rw-r--r-- | spec/javascripts/diffs/components/compare_versions_spec.js | 20 | ||||
| -rw-r--r-- | spec/javascripts/diffs/components/diff_file_header_spec.js | 4 | ||||
| -rw-r--r-- | spec/javascripts/lib/utils/common_utils_spec.js | 6 |
14 files changed, 179 insertions, 87 deletions
diff --git a/app/assets/javascripts/breakpoints.js b/app/assets/javascripts/breakpoints.js index 7951348d8b2..4d5d6bb864b 100644 --- a/app/assets/javascripts/breakpoints.js +++ b/app/assets/javascripts/breakpoints.js @@ -14,6 +14,9 @@ const BreakpointInstance = { return breakpoint; }, + isDesktop() { + return ['lg', 'md'].includes(this.getBreakpointSize); + }, }; export default BreakpointInstance; diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index e8f8c09152a..5e74998579b 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -20,6 +20,7 @@ import { MAX_TREE_WIDTH, TREE_HIDE_STATS_WIDTH, MR_TREE_SHOW_KEY, + CENTERED_LIMITED_CONTAINER_CLASSES, } from '../constants'; export default { @@ -114,6 +115,9 @@ export default { hideFileStats() { return this.treeWidth <= TREE_HIDE_STATS_WIDTH; }, + isLimitedContainer() { + return !this.showTreeList && !this.isParallelView; + }, }, watch: { diffViewType() { @@ -148,6 +152,7 @@ export default { this.adjustView(); eventHub.$once('fetchedNotesData', this.setDiscussions); eventHub.$once('fetchDiffData', this.fetchData); + this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES; }, beforeDestroy() { eventHub.$off('fetchDiffData', this.fetchData); @@ -202,8 +207,6 @@ export default { adjustView() { if (this.shouldShow) { this.$nextTick(() => { - window.mrTabs.resetViewContainer(); - window.mrTabs.expandViewContainer(this.showTreeList); this.setEventListeners(); }); } else { @@ -256,6 +259,7 @@ export default { :merge-request-diffs="mergeRequestDiffs" :merge-request-diff="mergeRequestDiff" :target-branch="targetBranch" + :is-limited-container="isLimitedContainer" /> <hidden-files-warning @@ -285,7 +289,12 @@ export default { /> <tree-list :hide-file-stats="hideFileStats" /> </div> - <div class="diff-files-holder"> + <div + class="diff-files-holder" + :class="{ + [CENTERED_LIMITED_CONTAINER_CLASSES]: isLimitedContainer, + }" + > <commit-widget v-if="commit" :commit="commit" /> <template v-if="renderDiffFiles"> <diff-file diff --git a/app/assets/javascripts/diffs/components/compare_versions.vue b/app/assets/javascripts/diffs/components/compare_versions.vue index fe49dfff10b..363ebad1594 100644 --- a/app/assets/javascripts/diffs/components/compare_versions.vue +++ b/app/assets/javascripts/diffs/components/compare_versions.vue @@ -7,6 +7,7 @@ import Icon from '~/vue_shared/components/icon.vue'; import CompareVersionsDropdown from './compare_versions_dropdown.vue'; import SettingsDropdown from './settings_dropdown.vue'; import DiffStats from './diff_stats.vue'; +import { CENTERED_LIMITED_CONTAINER_CLASSES } from '../constants'; export default { components: { @@ -35,6 +36,11 @@ export default { required: false, default: null, }, + isLimitedContainer: { + type: Boolean, + required: false, + default: false, + }, }, computed: { ...mapGetters('diffs', ['hasCollapsedFile', 'diffFilesLength']), @@ -62,6 +68,9 @@ export default { return this.mergeRequestDiff.base_version_path; }, }, + created() { + this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES; + }, mounted() { polyfillSticky(this.$el); }, @@ -77,8 +86,13 @@ export default { </script> <template> - <div class="mr-version-controls" :class="{ 'is-fileTreeOpen': showTreeList }"> - <div class="mr-version-menus-container content-block"> + <div class="mr-version-controls border-top border-bottom"> + <div + class="mr-version-menus-container content-block" + :class="{ + [CENTERED_LIMITED_CONTAINER_CLASSES]: isLimitedContainer, + }" + > <button v-gl-tooltip.hover type="button" diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue index fda7b7c5fd9..32e5fa5bf8b 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -1,7 +1,7 @@ <script> import _ from 'underscore'; import { mapActions, mapGetters } from 'vuex'; -import { polyfillSticky } from '~/lib/utils/sticky'; +import { polyfillSticky, stickyMonitor } from '~/lib/utils/sticky'; import ClipboardButton from '~/vue_shared/components/clipboard_button.vue'; import Icon from '~/vue_shared/components/icon.vue'; import FileIcon from '~/vue_shared/components/file_icon.vue'; @@ -11,7 +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'; +import { scrollToElement, contentTop } from '~/lib/utils/common_utils'; export default { components: { @@ -137,9 +137,20 @@ export default { isModeChanged() { return this.diffFile.viewer.name === diffViewerModes.mode_changed; }, + showExpandDiffToFullFileEnabled() { + return gon.features.expandDiffFullFile && !this.diffFile.is_fully_expanded; + }, + expandDiffToFullFileTitle() { + if (this.diffFile.isShowingFullFile) { + return s__('MRDiff|Show changes only'); + } + return s__('MRDiff|Show full file'); + }, }, mounted() { polyfillSticky(this.$refs.header); + const fileHeaderHeight = this.$refs.header.clientHeight; + stickyMonitor(this.$refs.header, contentTop() - fileHeaderHeight - 1, false); }, methods: { ...mapActions('diffs', ['toggleFileDiscussions', 'toggleFullDiff']), @@ -243,70 +254,70 @@ export default { class="file-actions d-none d-sm-block" > <diff-stats :added-lines="diffFile.added_lines" :removed-lines="diffFile.removed_lines" /> - <template v-if="diffFile.blob && diffFile.blob.readable_text"> - <button - :disabled="!diffHasDiscussions(diffFile)" - :class="{ active: hasExpandedDiscussions }" - :title="s__('MergeRequests|Toggle comments for this file')" - class="js-btn-vue-toggle-comments btn" - type="button" - @click="handleToggleDiscussions" - > - <icon name="comment" /> - </button> - - <edit-button - v-if="!diffFile.deleted_file" - :can-current-user-fork="canCurrentUserFork" - :edit-path="diffFile.edit_path" - :can-modify-blob="diffFile.can_modify_blob" - @showForkMessage="showForkMessage" - /> - </template> + <div class="btn-group" role="group"> + <template v-if="diffFile.blob && diffFile.blob.readable_text"> + <button + :disabled="!diffHasDiscussions(diffFile)" + :class="{ active: hasExpandedDiscussions }" + :title="s__('MergeRequests|Toggle comments for this file')" + class="js-btn-vue-toggle-comments btn" + type="button" + @click="handleToggleDiscussions" + > + <icon name="comment" /> + </button> - <a - v-if="diffFile.replaced_view_path" - :href="diffFile.replaced_view_path" - class="btn view-file js-view-replaced-file" - v-html="viewReplacedFileButtonText" - > - </a> - <gl-tooltip :target="() => $refs.viewButton" placement="bottom"> - <span v-html="viewFileButtonText"></span> - </gl-tooltip> - <gl-button - ref="viewButton" - :href="diffFile.view_path" - target="blank" - class="view-file js-view-file-button" - > - <icon name="external-link" /> - </gl-button> - <gl-button - v-if="!diffFile.is_fully_expanded" - class="expand-file js-expand-file" - @click="toggleFullDiff(diffFile.file_path)" - > - <template v-if="diffFile.isShowingFullFile"> - {{ s__('MRDiff|Show changes only') }} - </template> - <template v-else> - {{ s__('MRDiff|Show full file') }} + <edit-button + v-if="!diffFile.deleted_file" + :can-current-user-fork="canCurrentUserFork" + :edit-path="diffFile.edit_path" + :can-modify-blob="diffFile.can_modify_blob" + @showForkMessage="showForkMessage" + /> </template> - <gl-loading-icon v-if="diffFile.isLoadingFullFile" inline /> - </gl-button> - <a - v-if="diffFile.external_url" - v-gl-tooltip.hover - :href="diffFile.external_url" - :title="`View on ${diffFile.formatted_external_url}`" - target="_blank" - rel="noopener noreferrer" - class="btn btn-file-option js-external-url" - > - <icon name="external-link" /> - </a> + <a + v-if="diffFile.replaced_view_path" + :href="diffFile.replaced_view_path" + class="btn view-file js-view-replaced-file" + v-html="viewReplacedFileButtonText" + > + </a> + <gl-button + v-if="!diffFile.is_fully_expanded" + ref="expandDiffToFullFileButton" + v-gl-tooltip.hover + :title="expandDiffToFullFileTitle" + class="expand-file js-expand-file" + @click="toggleFullDiff(diffFile.file_path)" + > + <gl-loading-icon v-if="diffFile.isLoadingFullFile" color="dark" inline /> + <icon v-else-if="diffFile.isShowingFullFile" name="doc-changes" /> + <icon v-else name="doc-expand" /> + </gl-button> + <gl-button + ref="viewButton" + v-gl-tooltip.hover + :href="diffFile.view_path" + target="blank" + class="view-file js-view-file-button" + :title="viewFileButtonText" + > + <icon name="external-link" /> + </gl-button> + + <a + v-if="diffFile.external_url" + v-gl-tooltip.hover + :href="diffFile.external_url" + :title="`View on ${diffFile.formatted_external_url}`" + target="_blank" + rel="noopener noreferrer" + class="btn btn-file-option js-external-url" + > + <icon name="external-link" /> + </a> + </div> </div> </div> </template> diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index 6f380fe6ece..5dabe224baa 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -47,3 +47,6 @@ export const OLD_LINE_KEY = 'old_line'; export const NEW_LINE_KEY = 'new_line'; export const TYPE_KEY = 'type'; export const LEFT_LINE_KEY = 'left'; + +export const CENTERED_LIMITED_CONTAINER_CLASSES = + 'container-limited limit-container-width mx-auto px-3'; diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index 59930f8d4a3..2906604da57 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -7,7 +7,7 @@ import axios from './axios_utils'; import { getLocationHash } from './url_utility'; import { convertToCamelCase } from './text_utility'; import { isObject } from './type_utility'; -import BreakpointInstance from '../../breakpoints'; +import breakpointInstance from '../../breakpoints'; export const getPagePath = (index = 0) => { const page = $('body').attr('data-page') || ''; @@ -198,11 +198,10 @@ export const contentTop = () => { 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 isDesktop = breakpointInstance.isDesktop(); const diffFileTitleBar = - (mdScreenOrBigger && $('.diff-file .file-title-flex-parent:visible').outerHeight()) || 0; - const compareVersionsHeaderHeight = - (mdScreenOrBigger && $('.mr-version-controls').outerHeight()) || 0; + (isDesktop && $('.diff-file .file-title-flex-parent:visible').outerHeight()) || 0; + const compareVersionsHeaderHeight = (isDesktop && $('.mr-version-controls').outerHeight()) || 0; return ( perfBar + diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index 8d38310e8e6..53d3645cd63 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -331,6 +331,10 @@ span.idiff { padding: 5px $gl-padding; margin: 0; border-radius: $border-radius-default $border-radius-default 0 0; + + &.is-stuck { + border-radius: 0; + } } .file-header-content { diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index ebc5226bf87..5ea96392afa 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -7,12 +7,15 @@ cursor: pointer; @media (min-width: map-get($grid-breakpoints, md)) { - $mr-file-header-top: $mr-version-controls-height + $header-height + $mr-tabs-height; + // The `-1` below is to prevent two borders from clashing up against eachother - + // the bottom of the compare-versions header and the top of the file header + $mr-file-header-top: $mr-version-controls-height + $header-height + $mr-tabs-height - 1; position: -webkit-sticky; position: sticky; top: $mr-file-header-top; z-index: 102; + height: $mr-version-controls-height; &::before { content: ''; @@ -54,7 +57,8 @@ background-color: $gray-normal; } - a { + a, + button { color: $gray-700; } diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index f7ebd84dc1c..86b58c1b1b2 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -736,7 +736,6 @@ background: $gray-light; color: $gl-text-color; margin-top: -1px; - border-top: 1px solid $border-color; .mr-version-menus-container { display: flex; @@ -759,6 +758,7 @@ .content-block { padding: $gl-padding-top $gl-padding; + border-bottom: 0; } .comments-disabled-notif { @@ -783,16 +783,18 @@ padding-right: 5px; } + // Shortening button height by 1px to make compare-versions + // header 56px and fit into our 8px design grid + button { + height: 34px; + } + @include media-breakpoint-up(md) { position: -webkit-sticky; position: sticky; top: $header-height + $mr-tabs-height; - width: 100%; - - &.is-fileTreeOpen { - margin-left: -16px; - width: calc(100% + 32px); - } + margin-left: -16px; + width: calc(100% + 32px); .mr-version-menus-container { flex-wrap: nowrap; diff --git a/changelogs/unreleased/57364-improve-diff-nav-header.yml b/changelogs/unreleased/57364-improve-diff-nav-header.yml new file mode 100644 index 00000000000..95d119b949c --- /dev/null +++ b/changelogs/unreleased/57364-improve-diff-nav-header.yml @@ -0,0 +1,5 @@ +--- +title: Make stylistic improvements to diff nav header +merge_request: 26557 +author: +type: fixed diff --git a/spec/javascripts/diffs/components/app_spec.js b/spec/javascripts/diffs/components/app_spec.js index 8d7c52a2876..3ce69bc3c20 100644 --- a/spec/javascripts/diffs/components/app_spec.js +++ b/spec/javascripts/diffs/components/app_spec.js @@ -57,6 +57,24 @@ describe('diffs/components/app', () => { wrapper.destroy(); }); + it('adds container-limiting classes when showFileTree is false with inline diffs', () => { + createComponent({}, ({ state }) => { + state.diffs.showTreeList = false; + state.diffs.isParallelView = false; + }); + + expect(wrapper.contains('.container-limited.limit-container-width')).toBe(true); + }); + + it('does not add container-limiting classes when showFileTree is false with inline diffs', () => { + createComponent({}, ({ state }) => { + state.diffs.showTreeList = true; + state.diffs.isParallelView = false; + }); + + expect(wrapper.contains('.container-limited.limit-container-width')).toBe(false); + }); + it('displays loading icon on loading', () => { createComponent({}, ({ state }) => { state.diffs.isLoading = true; diff --git a/spec/javascripts/diffs/components/compare_versions_spec.js b/spec/javascripts/diffs/components/compare_versions_spec.js index e886f962d2f..77f8352047c 100644 --- a/spec/javascripts/diffs/components/compare_versions_spec.js +++ b/spec/javascripts/diffs/components/compare_versions_spec.js @@ -66,6 +66,26 @@ describe('CompareVersions', () => { expect(inlineBtn.innerHTML).toContain('Inline'); expect(parallelBtn.innerHTML).toContain('Side-by-side'); }); + + it('adds container-limiting classes when showFileTree is false with inline diffs', () => { + vm.isLimitedContainer = true; + + vm.$nextTick(() => { + const limitedContainer = vm.$el.querySelector('.container-limited.limit-container-width'); + + expect(limitedContainer).not.toBeNull(); + }); + }); + + it('does not add container-limiting classes when showFileTree is false with inline diffs', () => { + vm.isLimitedContainer = false; + + vm.$nextTick(() => { + const limitedContainer = vm.$el.querySelector('.container-limited.limit-container-width'); + + expect(limitedContainer).toBeNull(); + }); + }); }); describe('setInlineDiffViewType', () => { diff --git a/spec/javascripts/diffs/components/diff_file_header_spec.js b/spec/javascripts/diffs/components/diff_file_header_spec.js index 6614069f44d..e1170c9762e 100644 --- a/spec/javascripts/diffs/components/diff_file_header_spec.js +++ b/spec/javascripts/diffs/components/diff_file_header_spec.js @@ -672,7 +672,7 @@ describe('diff_file_header', () => { vm = mountComponentWithStore(Component, { props, store }); - expect(vm.$el.querySelector('.js-expand-file').textContent).toContain('Show changes only'); + expect(vm.$el.querySelector('.ic-doc-changes')).not.toBeNull(); }); it('shows expand text', () => { @@ -680,7 +680,7 @@ describe('diff_file_header', () => { vm = mountComponentWithStore(Component, { props, store }); - expect(vm.$el.querySelector('.js-expand-file').textContent).toContain('Show full file'); + expect(vm.$el.querySelector('.ic-doc-expand')).not.toBeNull(); }); it('renders loading icon', () => { diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js index 2084c36e484..da012e1d5f7 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js +++ b/spec/javascripts/lib/utils/common_utils_spec.js @@ -2,7 +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'; +import breakpointInstance from '~/breakpoints'; const PIXEL_TOLERANCE = 0.2; @@ -383,7 +383,7 @@ 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'); + spyOn(breakpointInstance, 'isDesktop').and.returnValue(false); setFixtures(` <div class="diff-file file-title-flex-parent"> @@ -398,7 +398,7 @@ describe('common_utils', () => { }); it('adds height for fileTitle and compareVersionsHeader screen is large enough', () => { - spyOn(BreakpointInstance, 'getBreakpointSize').and.returnValue('lg'); + spyOn(breakpointInstance, 'isDesktop').and.returnValue(true); setFixtures(` <div class="diff-file file-title-flex-parent"> |