summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSam Bigelow <sbigelow@gitlab.com>2019-03-28 17:37:06 -0400
committerSam Bigelow <sbigelow@gitlab.com>2019-04-05 14:57:01 -0400
commitbf47270e9072fccc1bc9575b4d70b9d5c8ac021f (patch)
tree4379be730ea34a581437acf70f096a9c9fe7d544
parent361c7ca69787e75b3ad2c776245452bd49e4a13c (diff)
downloadgitlab-ce-bf47270e9072fccc1bc9575b4d70b9d5c8ac021f.tar.gz
Improve diff navigation header
- Compare versions header is full width except in the unified diff mode with no tree sidebar - Bar is always full width, but the content within stays centered when unified and no tree sidebar - File header is the same height as the "Compare versions header" - aligns with the design system grid guidelines => 56px - Diff file headers use a button group, switch icon order to open file externally being the last option, all buttons will become icon buttons (icon delivery by @dimitrieh) - If a file header becomes sticky no rounded corner/double border problem is visible anymore
-rw-r--r--app/assets/javascripts/breakpoints.js3
-rw-r--r--app/assets/javascripts/diffs/components/app.vue15
-rw-r--r--app/assets/javascripts/diffs/components/compare_versions.vue18
-rw-r--r--app/assets/javascripts/diffs/components/diff_file_header.vue137
-rw-r--r--app/assets/javascripts/diffs/constants.js3
-rw-r--r--app/assets/javascripts/lib/utils/common_utils.js9
-rw-r--r--app/assets/stylesheets/framework/files.scss4
-rw-r--r--app/assets/stylesheets/pages/diff.scss8
-rw-r--r--app/assets/stylesheets/pages/merge_requests.scss16
-rw-r--r--changelogs/unreleased/57364-improve-diff-nav-header.yml5
-rw-r--r--spec/javascripts/diffs/components/app_spec.js18
-rw-r--r--spec/javascripts/diffs/components/compare_versions_spec.js20
-rw-r--r--spec/javascripts/diffs/components/diff_file_header_spec.js4
-rw-r--r--spec/javascripts/lib/utils/common_utils_spec.js6
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">