kotovalexarian-likes-gitlab
/
gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

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
This commit is contained in:
Sam Bigelow 2019-03-28 17:37:06 -04:00
parent 361c7ca697
commit bf47270e90
14 changed files with 180 additions and 88 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
app/assets/javascripts/breakpoints.js
View File

@ -14,6 +14,9 @@ const BreakpointInstance = {
return breakpoint;
},
isDesktop() {
return ['lg', 'md'].includes(this.getBreakpointSize);
},
};
export default BreakpointInstance;

15
app/assets/javascripts/diffs/components/app.vue
View File

@ -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

18
app/assets/javascripts/diffs/components/compare_versions.vue
View File

@ -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"

139
app/assets/javascripts/diffs/components/diff_file_header.vue
View File

@ -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"
<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>
<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>
<a
v-if="diffFile.replaced_view_path"
:href="diffFile.replaced_view_path"
class="btn view-file js-view-replaced-file"
v-html="viewReplacedFileButtonText"
>
<icon name="comment" />
</button>
</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>
<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>
<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') }}
</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.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>

3
app/assets/javascripts/diffs/constants.js
View File

@ -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';

9
app/assets/javascripts/lib/utils/common_utils.js
View File

@ -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 +

4
app/assets/stylesheets/framework/files.scss
View File

@ -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 {

8
app/assets/stylesheets/pages/diff.scss
View File

@ -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;
}

16
app/assets/stylesheets/pages/merge_requests.scss
View File

@ -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;

5
changelogs/unreleased/57364-improve-diff-nav-header.yml Normal file
View File

@ -0,0 +1,5 @@
---
title: Make stylistic improvements to diff nav header
merge_request: 26557
author:
type: fixed

18
spec/javascripts/diffs/components/app_spec.js
View File

@ -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;

20
spec/javascripts/diffs/components/compare_versions_spec.js
View File

@ -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', () => {

4
spec/javascripts/diffs/components/diff_file_header_spec.js
View File

@ -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', () => {

6
spec/javascripts/lib/utils/common_utils_spec.js
View File

@ -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">