Commit d6c42a8b authored by Thomas Randolph's avatar Thomas Randolph Committed by Phil Hughes

Add `manuallyCollapsed` flag to Diff Files

Of course, this means that there's a bunch of logic
to coordinate the new flag with the original -
`automaticallyCollapsed` - flag.

In addition: the UI is updated to use the new flag
whenever it's been set, but to continue to fall back
to the old flag. This is primarily facilitated by a new
helper utility for diff files to do the resolution in a
single centralized place.

Of note: the diff_file is updated to depend on the
global store as the authoritative source of determining
whether the file is collapsed or not. The previous
version kept a local `isCollapsed` in sync with the
global store, which introduced a huge amount of
interwoven complexity. The update ingests the
resolved collapsed state from the store when
various observed properties update instead of
inverting that flow.

This is a full refactor of file collapsing.
Because the previous version was a single flag, then
modified with a user preference (file-by-file), there
were some issues including a THIRD possible way
that collapsing works. This update slightly simplifies
how collapsing works by depending more heavily
on the two flags stored in state and only modifying
them with the user preference when necessary.
parent 98c9897a
...@@ -124,7 +124,6 @@ export default { ...@@ -124,7 +124,6 @@ export default {
return { return {
treeWidth, treeWidth,
diffFilesLength: 0, diffFilesLength: 0,
collapsedWarningDismissed: false,
}; };
}, },
computed: { computed: {
...@@ -153,7 +152,7 @@ export default { ...@@ -153,7 +152,7 @@ export default {
'canMerge', 'canMerge',
'hasConflicts', 'hasConflicts',
]), ]),
...mapGetters('diffs', ['hasCollapsedFile', 'isParallelView', 'currentDiffIndex']), ...mapGetters('diffs', ['whichCollapsedTypes', 'isParallelView', 'currentDiffIndex']),
...mapGetters(['isNotesFetched', 'getNoteableData']), ...mapGetters(['isNotesFetched', 'getNoteableData']),
diffs() { diffs() {
if (!this.viewDiffsFileByFile) { if (!this.viewDiffsFileByFile) {
...@@ -206,11 +205,7 @@ export default { ...@@ -206,11 +205,7 @@ export default {
visible = this.$options.alerts.ALERT_OVERFLOW_HIDDEN; visible = this.$options.alerts.ALERT_OVERFLOW_HIDDEN;
} else if (this.isDiffHead && this.hasConflicts) { } else if (this.isDiffHead && this.hasConflicts) {
visible = this.$options.alerts.ALERT_MERGE_CONFLICT; visible = this.$options.alerts.ALERT_MERGE_CONFLICT;
} else if ( } else if (this.whichCollapsedTypes.automatic && !this.viewDiffsFileByFile) {
this.hasCollapsedFile &&
!this.collapsedWarningDismissed &&
!this.viewDiffsFileByFile
) {
visible = this.$options.alerts.ALERT_COLLAPSED_FILES; visible = this.$options.alerts.ALERT_COLLAPSED_FILES;
} }
...@@ -429,9 +424,6 @@ export default { ...@@ -429,9 +424,6 @@ export default {
this.toggleShowTreeList(false); this.toggleShowTreeList(false);
} }
}, },
dismissCollapsedWarning() {
this.collapsedWarningDismissed = true;
},
}, },
minTreeWidth: MIN_TREE_WIDTH, minTreeWidth: MIN_TREE_WIDTH,
maxTreeWidth: MAX_TREE_WIDTH, maxTreeWidth: MAX_TREE_WIDTH,
...@@ -464,7 +456,6 @@ export default { ...@@ -464,7 +456,6 @@ export default {
<collapsed-files-warning <collapsed-files-warning
v-if="visibleWarning == $options.alerts.ALERT_COLLAPSED_FILES" v-if="visibleWarning == $options.alerts.ALERT_COLLAPSED_FILES"
:limited="isLimitedContainer" :limited="isLimitedContainer"
@dismiss="dismissCollapsedWarning"
/> />
<div <div
......
...@@ -38,7 +38,7 @@ export default { ...@@ -38,7 +38,7 @@ export default {
}, },
computed: { computed: {
...mapGetters('diffs', [ ...mapGetters('diffs', [
'hasCollapsedFile', 'whichCollapsedTypes',
'diffCompareDropdownTargetVersions', 'diffCompareDropdownTargetVersions',
'diffCompareDropdownSourceVersions', 'diffCompareDropdownSourceVersions',
]), ]),
...@@ -129,7 +129,7 @@ export default { ...@@ -129,7 +129,7 @@ export default {
{{ __('Show latest version') }} {{ __('Show latest version') }}
</gl-button> </gl-button>
<gl-button <gl-button
v-show="hasCollapsedFile" v-show="whichCollapsedTypes.any"
variant="default" variant="default"
class="gl-mr-3" class="gl-mr-3"
@click="expandAllFiles" @click="expandAllFiles"
......
...@@ -10,6 +10,8 @@ import eventHub from '../../notes/event_hub'; ...@@ -10,6 +10,8 @@ import eventHub from '../../notes/event_hub';
import DiffFileHeader from './diff_file_header.vue'; import DiffFileHeader from './diff_file_header.vue';
import DiffContent from './diff_content.vue'; import DiffContent from './diff_content.vue';
import { diffViewerErrors } from '~/ide/constants'; import { diffViewerErrors } from '~/ide/constants';
import { collapsedType, isCollapsed } from '../diff_file';
import { DIFF_FILE_AUTOMATIC_COLLAPSE, DIFF_FILE_MANUAL_COLLAPSE } from '../constants';
export default { export default {
components: { components: {
...@@ -44,7 +46,7 @@ export default { ...@@ -44,7 +46,7 @@ export default {
return { return {
isLoadingCollapsedDiff: false, isLoadingCollapsedDiff: false,
forkMessageVisible: false, forkMessageVisible: false,
isCollapsed: this.file.viewer.automaticallyCollapsed || false, isCollapsed: isCollapsed(this.file),
}; };
}, },
computed: { computed: {
...@@ -71,7 +73,7 @@ export default { ...@@ -71,7 +73,7 @@ export default {
return this.file.viewer.error === diffViewerErrors.too_large; return this.file.viewer.error === diffViewerErrors.too_large;
}, },
errorMessage() { errorMessage() {
return this.file.viewer.error_message; return !this.manuallyCollapsed ? this.file.viewer.error_message : '';
}, },
forkMessage() { forkMessage() {
return sprintf( return sprintf(
...@@ -85,57 +87,94 @@ export default { ...@@ -85,57 +87,94 @@ export default {
false, false,
); );
}, },
}, hasBodyClasses() {
watch: { const domParts = {
isCollapsed: function fileCollapsedWatch(newVal, oldVal) { header: 'gl-rounded-base!',
if (!newVal && oldVal && !this.hasDiff) { contentByHash: '',
this.handleLoadCollapsedDiff(); content: '',
};
if (this.showBody) {
domParts.header = 'gl-rounded-bottom-left-none gl-rounded-bottom-right-none';
domParts.contentByHash =
'gl-rounded-none gl-rounded-bottom-left-base gl-rounded-bottom-right-base gl-border-1 gl-border-t-0! gl-border-solid gl-border-gray-100';
domParts.content = 'gl-rounded-bottom-left-base gl-rounded-bottom-right-base';
} }
this.setFileCollapsed({ filePath: this.file.file_path, collapsed: newVal }); return domParts;
},
automaticallyCollapsed() {
return collapsedType(this.file) === DIFF_FILE_AUTOMATIC_COLLAPSE;
},
manuallyCollapsed() {
return collapsedType(this.file) === DIFF_FILE_MANUAL_COLLAPSE;
},
showBody() {
return !this.isCollapsed || this.automaticallyCollapsed;
}, },
showWarning() {
return this.isCollapsed && (this.automaticallyCollapsed && !this.viewDiffsFileByFile);
},
showContent() {
return !this.isCollapsed && !this.isFileTooLarge;
},
},
watch: {
'file.file_hash': { 'file.file_hash': {
handler: function watchFileHash() { handler: function hashChangeWatch(newHash, oldHash) {
if (this.viewDiffsFileByFile && this.file.viewer.automaticallyCollapsed) { this.isCollapsed = isCollapsed(this.file);
this.isCollapsed = false;
this.handleLoadCollapsedDiff(); if (newHash && oldHash && !this.hasDiff) {
} else { this.requestDiff();
this.isCollapsed = this.file.viewer.automaticallyCollapsed || false;
} }
}, },
immediate: true, immediate: true,
}, },
'file.viewer.automaticallyCollapsed': function setIsCollapsed(newVal) { 'file.viewer.automaticallyCollapsed': {
if (!this.viewDiffsFileByFile) { handler: function autoChangeWatch(automaticValue) {
this.isCollapsed = newVal; if (collapsedType(this.file) !== DIFF_FILE_MANUAL_COLLAPSE) {
} this.isCollapsed = this.viewDiffsFileByFile ? false : automaticValue;
}
},
immediate: true,
},
'file.viewer.manuallyCollapsed': {
handler: function manualChangeWatch(manualValue) {
if (manualValue !== null) {
this.isCollapsed = manualValue;
}
},
immediate: true,
}, },
}, },
created() { created() {
eventHub.$on(`loadCollapsedDiff/${this.file.file_hash}`, this.handleLoadCollapsedDiff); eventHub.$on(`loadCollapsedDiff/${this.file.file_hash}`, this.requestDiff);
}, },
methods: { methods: {
...mapActions('diffs', [ ...mapActions('diffs', [
'loadCollapsedDiff', 'loadCollapsedDiff',
'assignDiscussionsToDiff', 'assignDiscussionsToDiff',
'setRenderIt', 'setRenderIt',
'setFileCollapsed', 'setFileCollapsedByUser',
]), ]),
handleToggle() { handleToggle() {
if (!this.hasDiff) { const currentCollapsedFlag = this.isCollapsed;
this.handleLoadCollapsedDiff();
} else { this.setFileCollapsedByUser({
this.isCollapsed = !this.isCollapsed; filePath: this.file.file_path,
this.setRenderIt(this.file); collapsed: !currentCollapsedFlag,
});
if (!this.hasDiff && currentCollapsedFlag) {
this.requestDiff();
} }
}, },
handleLoadCollapsedDiff() { requestDiff() {
this.isLoadingCollapsedDiff = true; this.isLoadingCollapsedDiff = true;
this.loadCollapsedDiff(this.file) this.loadCollapsedDiff(this.file)
.then(() => { .then(() => {
this.isLoadingCollapsedDiff = false; this.isLoadingCollapsedDiff = false;
this.isCollapsed = false;
this.setRenderIt(this.file); this.setRenderIt(this.file);
}) })
.then(() => { .then(() => {
...@@ -167,9 +206,10 @@ export default { ...@@ -167,9 +206,10 @@ export default {
:class="{ :class="{
'is-active': currentDiffFileId === file.file_hash, 'is-active': currentDiffFileId === file.file_hash,
'comments-disabled': Boolean(file.brokenSymlink), 'comments-disabled': Boolean(file.brokenSymlink),
'has-body': showBody,
}" }"
:data-path="file.new_path" :data-path="file.new_path"
class="diff-file file-holder" class="diff-file file-holder gl-border-none"
> >
<diff-file-header <diff-file-header
:can-current-user-fork="canCurrentUserFork" :can-current-user-fork="canCurrentUserFork"
...@@ -178,7 +218,8 @@ export default { ...@@ -178,7 +218,8 @@ export default {
:expanded="!isCollapsed" :expanded="!isCollapsed"
:add-merge-request-buttons="true" :add-merge-request-buttons="true"
:view-diffs-file-by-file="viewDiffsFileByFile" :view-diffs-file-by-file="viewDiffsFileByFile"
class="js-file-title file-title" class="js-file-title file-title gl-border-1 gl-border-solid gl-border-gray-100"
:class="hasBodyClasses.header"
@toggleFile="handleToggle" @toggleFile="handleToggle"
@showForkMessage="showForkMessage" @showForkMessage="showForkMessage"
/> />
...@@ -198,21 +239,35 @@ export default { ...@@ -198,21 +239,35 @@ export default {
{{ __('Cancel') }} {{ __('Cancel') }}
</button> </button>
</div> </div>
<gl-loading-icon v-if="showLoadingIcon" class="diff-content loading" />
<template v-else> <template v-else>
<div :id="`diff-content-${file.file_hash}`"> <div
<div v-if="errorMessage" class="diff-viewer"> :id="`diff-content-${file.file_hash}`"
:class="hasBodyClasses.contentByHash"
data-testid="content-area"
>
<gl-loading-icon
v-if="showLoadingIcon"
class="diff-content loading gl-my-0 gl-pt-3"
data-testid="loader-icon"
/>
<div v-else-if="errorMessage" class="diff-viewer">
<div v-safe-html="errorMessage" class="nothing-here-block"></div> <div v-safe-html="errorMessage" class="nothing-here-block"></div>
</div> </div>
<template v-else> <template v-else>
<div v-show="isCollapsed" class="nothing-here-block diff-collapsed"> <div v-show="showWarning" class="nothing-here-block diff-collapsed">
{{ __('This diff is collapsed.') }} {{ __('This diff is collapsed.') }}
<a class="click-to-expand js-click-to-expand" href="#" @click.prevent="handleToggle">{{ <a
__('Click to expand it.') class="click-to-expand"
}}</a> data-testid="toggle-link"
href="#"
@click.prevent="handleToggle"
>
{{ __('Click to expand it.') }}
</a>
</div> </div>
<diff-content <diff-content
v-show="!isCollapsed && !isFileTooLarge" v-show="showContent"
:class="hasBodyClasses.content"
:diff-file="file" :diff-file="file"
:help-page-path="helpPagePath" :help-page-path="helpPagePath"
/> />
......
...@@ -18,6 +18,7 @@ import { __, s__, sprintf } from '~/locale'; ...@@ -18,6 +18,7 @@ import { __, s__, sprintf } from '~/locale';
import { diffViewerModes } from '~/ide/constants'; import { diffViewerModes } from '~/ide/constants';
import DiffStats from './diff_stats.vue'; import DiffStats from './diff_stats.vue';
import { scrollToElement } from '~/lib/utils/common_utils'; import { scrollToElement } from '~/lib/utils/common_utils';
import { isCollapsed } from '../diff_file';
import { DIFF_FILE_HEADER } from '../i18n'; import { DIFF_FILE_HEADER } from '../i18n';
export default { export default {
...@@ -125,6 +126,9 @@ export default { ...@@ -125,6 +126,9 @@ export default {
isUsingLfs() { isUsingLfs() {
return this.diffFile.stored_externally && this.diffFile.external_storage === 'lfs'; return this.diffFile.stored_externally && this.diffFile.external_storage === 'lfs';
}, },
isCollapsed() {
return isCollapsed(this.diffFile, { fileByFile: this.viewDiffsFileByFile });
},
collapseIcon() { collapseIcon() {
return this.expanded ? 'chevron-down' : 'chevron-right'; return this.expanded ? 'chevron-down' : 'chevron-right';
}, },
...@@ -334,7 +338,7 @@ export default { ...@@ -334,7 +338,7 @@ export default {
</gl-dropdown-item> </gl-dropdown-item>
</template> </template>
<template v-if="!diffFile.viewer.automaticallyCollapsed"> <template v-if="!isCollapsed">
<gl-dropdown-divider <gl-dropdown-divider
v-if="!diffFile.is_fully_expanded || diffHasDiscussions(diffFile)" v-if="!diffFile.is_fully_expanded || diffHasDiscussions(diffFile)"
/> />
......
...@@ -73,6 +73,10 @@ export const ALERT_OVERFLOW_HIDDEN = 'overflow'; ...@@ -73,6 +73,10 @@ export const ALERT_OVERFLOW_HIDDEN = 'overflow';
export const ALERT_MERGE_CONFLICT = 'merge-conflict'; export const ALERT_MERGE_CONFLICT = 'merge-conflict';
export const ALERT_COLLAPSED_FILES = 'collapsed'; export const ALERT_COLLAPSED_FILES = 'collapsed';
// Diff File collapse types
export const DIFF_FILE_AUTOMATIC_COLLAPSE = 'automatic';
export const DIFF_FILE_MANUAL_COLLAPSE = 'manual';
// State machine states // State machine states
export const STATE_IDLING = 'idle'; export const STATE_IDLING = 'idle';
export const STATE_LOADING = 'loading'; export const STATE_LOADING = 'loading';
......
import { DIFF_FILE_SYMLINK_MODE, DIFF_FILE_DELETED_MODE } from './constants'; import {
DIFF_FILE_SYMLINK_MODE,
DIFF_FILE_DELETED_MODE,
DIFF_FILE_MANUAL_COLLAPSE,
DIFF_FILE_AUTOMATIC_COLLAPSE,
} from './constants';
function fileSymlinkInformation(file, fileList) { function fileSymlinkInformation(file, fileList) {
const duplicates = fileList.filter(iteratedFile => iteratedFile.file_hash === file.file_hash); const duplicates = fileList.filter(iteratedFile => iteratedFile.file_hash === file.file_hash);
...@@ -23,6 +28,7 @@ function collapsed(file) { ...@@ -23,6 +28,7 @@ function collapsed(file) {
return { return {
automaticallyCollapsed: viewer.automaticallyCollapsed || viewer.collapsed || false, automaticallyCollapsed: viewer.automaticallyCollapsed || viewer.collapsed || false,
manuallyCollapsed: null,
}; };
} }
...@@ -37,3 +43,19 @@ export function prepareRawDiffFile({ file, allFiles }) { ...@@ -37,3 +43,19 @@ export function prepareRawDiffFile({ file, allFiles }) {
return file; return file;
} }
export function collapsedType(file) {
const isManual = typeof file.viewer?.manuallyCollapsed === 'boolean';
return isManual ? DIFF_FILE_MANUAL_COLLAPSE : DIFF_FILE_AUTOMATIC_COLLAPSE;
}
export function isCollapsed(file) {
const type = collapsedType(file);
const collapsedStates = {
[DIFF_FILE_AUTOMATIC_COLLAPSE]: file.viewer?.automaticallyCollapsed || false,
[DIFF_FILE_MANUAL_COLLAPSE]: file.viewer?.manuallyCollapsed,
};
return collapsedStates[type];
}
...@@ -40,8 +40,11 @@ import { ...@@ -40,8 +40,11 @@ import {
DIFF_WHITESPACE_COOKIE_NAME, DIFF_WHITESPACE_COOKIE_NAME,
SHOW_WHITESPACE, SHOW_WHITESPACE,
NO_SHOW_WHITESPACE, NO_SHOW_WHITESPACE,
DIFF_FILE_MANUAL_COLLAPSE,
DIFF_FILE_AUTOMATIC_COLLAPSE,
} from '../constants'; } from '../constants';
import { diffViewerModes } from '~/ide/constants'; import { diffViewerModes } from '~/ide/constants';
import { isCollapsed } from '../diff_file';
export const setBaseConfig = ({ commit }, options) => { export const setBaseConfig = ({ commit }, options) => {
const { const {
...@@ -239,6 +242,13 @@ export const renderFileForDiscussionId = ({ commit, rootState, state }, discussi ...@@ -239,6 +242,13 @@ export const renderFileForDiscussionId = ({ commit, rootState, state }, discussi
if (file.viewer.automaticallyCollapsed) { if (file.viewer.automaticallyCollapsed) {
eventHub.$emit(`loadCollapsedDiff/${file.file_hash}`); eventHub.$emit(`loadCollapsedDiff/${file.file_hash}`);
scrollToElement(document.getElementById(file.file_hash)); scrollToElement(document.getElementById(file.file_hash));
} else if (file.viewer.manuallyCollapsed) {
commit(types.SET_FILE_COLLAPSED, {
filePath: file.file_path,
collapsed: false,
trigger: DIFF_FILE_AUTOMATIC_COLLAPSE,
});
eventHub.$emit('scrollToDiscussion');
} else { } else {
eventHub.$emit('scrollToDiscussion'); eventHub.$emit('scrollToDiscussion');
} }
...@@ -252,8 +262,7 @@ export const startRenderDiffsQueue = ({ state, commit }) => { ...@@ -252,8 +262,7 @@ export const startRenderDiffsQueue = ({ state, commit }) => {
const nextFile = state.diffFiles.find( const nextFile = state.diffFiles.find(
file => file =>
!file.renderIt && !file.renderIt &&
(file.viewer && (file.viewer && (!isCollapsed(file) || file.viewer.name !== diffViewerModes.text)),
(!file.viewer.automaticallyCollapsed || file.viewer.name !== diffViewerModes.text)),
); );
if (nextFile) { if (nextFile) {
...@@ -641,8 +650,9 @@ export function switchToFullDiffFromRenamedFile({ commit, dispatch, state }, { d ...@@ -641,8 +650,9 @@ export function switchToFullDiffFromRenamedFile({ commit, dispatch, state }, { d
}); });
} }
export const setFileCollapsed = ({ commit }, { filePath, collapsed }) => export const setFileCollapsedByUser = ({ commit }, { filePath, collapsed }) => {
commit(types.SET_FILE_COLLAPSED, { filePath, collapsed }); commit(types.SET_FILE_COLLAPSED, { filePath, collapsed, trigger: DIFF_FILE_MANUAL_COLLAPSE });
};
export const setSuggestPopoverDismissed = ({ commit, state }) => export const setSuggestPopoverDismissed = ({ commit, state }) =>
axios axios
......
...@@ -8,8 +8,16 @@ export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW ...@@ -8,8 +8,16 @@ export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW
export const isInlineView = state => state.diffViewType === INLINE_DIFF_VIEW_TYPE; export const isInlineView = state => state.diffViewType === INLINE_DIFF_VIEW_TYPE;
export const hasCollapsedFile = state => export const whichCollapsedTypes = state => {
state.diffFiles.some(file => file.viewer && file.viewer.automaticallyCollapsed); const automatic = state.diffFiles.some(file => file.viewer?.automaticallyCollapsed);
const manual = state.diffFiles.some(file => file.viewer?.manuallyCollapsed);
return {
any: automatic || manual,
automatic,
manual,
};
};
export const commitId = state => (state.commit && state.commit.id ? state.commit.id : null); export const commitId = state => (state.commit && state.commit.id ? state.commit.id : null);
......
import Vue from 'vue'; import Vue from 'vue';
import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
import { INLINE_DIFF_VIEW_TYPE } from '../constants'; import {
DIFF_FILE_MANUAL_COLLAPSE,
DIFF_FILE_AUTOMATIC_COLLAPSE,
INLINE_DIFF_VIEW_TYPE,
} from '../constants';
import { import {
findDiffFile, findDiffFile,
addLineReferences, addLineReferences,
...@@ -16,6 +20,12 @@ function updateDiffFilesInState(state, files) { ...@@ -16,6 +20,12 @@ function updateDiffFilesInState(state, files) {
return Object.assign(state, { diffFiles: files }); return Object.assign(state, { diffFiles: files });
} }
function renderFile(file) {
Object.assign(file, {
renderIt: true,
});
}
export default { export default {
[types.SET_BASE_CONFIG](state, options) { [types.SET_BASE_CONFIG](state, options) {
const { const {
...@@ -81,9 +91,7 @@ export default { ...@@ -81,9 +91,7 @@ export default {
}, },
[types.RENDER_FILE](state, file) { [types.RENDER_FILE](state, file) {
Object.assign(file, { renderFile(file);
renderIt: true,
});
}, },
[types.SET_MERGE_REQUEST_DIFFS](state, mergeRequestDiffs) { [types.SET_MERGE_REQUEST_DIFFS](state, mergeRequestDiffs) {
...@@ -173,6 +181,7 @@ export default { ...@@ -173,6 +181,7 @@ export default {
Object.assign(file, { Object.assign(file, {
viewer: Object.assign(file.viewer, { viewer: Object.assign(file.viewer, {
automaticallyCollapsed: false, automaticallyCollapsed: false,
manuallyCollapsed: false,
}), }),
}); });
}); });
...@@ -351,11 +360,24 @@ export default { ...@@ -351,11 +360,24 @@ export default {
file.isShowingFullFile = true; file.isShowingFullFile = true;
file.isLoadingFullFile = false; file.isLoadingFullFile = false;
}, },
[types.SET_FILE_COLLAPSED](state, { filePath, collapsed }) { [types.SET_FILE_COLLAPSED](
state,
{ filePath, collapsed, trigger = DIFF_FILE_AUTOMATIC_COLLAPSE },
) {
const file = state.diffFiles.find(f => f.file_path === filePath); const file = state.diffFiles.find(f => f.file_path === filePath);
if (file && file.viewer) { if (file && file.viewer) {
file.viewer.automaticallyCollapsed = collapsed; if (trigger === DIFF_FILE_MANUAL_COLLAPSE) {
file.viewer.automaticallyCollapsed = false;
file.viewer.manuallyCollapsed = collapsed;
} else if (trigger === DIFF_FILE_AUTOMATIC_COLLAPSE) {
file.viewer.automaticallyCollapsed = collapsed;
file.viewer.manuallyCollapsed = null;
}
}
if (file && !collapsed) {
renderFile(file);
} }
}, },
[types.SET_HIDDEN_VIEW_DIFF_FILE_LINES](state, { filePath, lines }) { [types.SET_HIDDEN_VIEW_DIFF_FILE_LINES](state, { filePath, lines }) {
......
...@@ -7,6 +7,7 @@ import DiffViewer from '~/vue_shared/components/diff_viewer/diff_viewer.vue'; ...@@ -7,6 +7,7 @@ import DiffViewer from '~/vue_shared/components/diff_viewer/diff_viewer.vue';
import ImageDiffOverlay from '~/diffs/components/image_diff_overlay.vue'; import ImageDiffOverlay from '~/diffs/components/image_diff_overlay.vue';
import { getDiffMode } from '~/diffs/store/utils'; import { getDiffMode } from '~/diffs/store/utils';
import { diffViewerModes } from '~/ide/constants'; import { diffViewerModes } from '~/ide/constants';
import { isCollapsed } from '../../diffs/diff_file';
const FIRST_CHAR_REGEX = /^(\+|-| )/; const FIRST_CHAR_REGEX = /^(\+|-| )/;
...@@ -46,6 +47,9 @@ export default { ...@@ -46,6 +47,9 @@ export default {
this.discussion.truncated_diff_lines && this.discussion.truncated_diff_lines.length !== 0 this.discussion.truncated_diff_lines && this.discussion.truncated_diff_lines.length !== 0
); );
}, },
isCollapsed() {
return isCollapsed(this.discussion.diff_file);
},
}, },
mounted() { mounted() {
if (this.isTextFile && !this.hasTruncatedDiffLines) { if (this.isTextFile && !this.hasTruncatedDiffLines) {
...@@ -76,7 +80,7 @@ export default { ...@@ -76,7 +80,7 @@ export default {
:discussion-path="discussion.discussion_path" :discussion-path="discussion.discussion_path"
:diff-file="discussion.diff_file" :diff-file="discussion.diff_file"
:can-current-user-fork="false" :can-current-user-fork="false"
:expanded="!discussion.diff_file.viewer.automaticallyCollapsed" :expanded="!isCollapsed"
/> />
<div v-if="isTextFile" class="diff-content"> <div v-if="isTextFile" class="diff-content">
<table class="code js-syntax-highlight" :class="$options.userColorSchemeClass"> <table class="code js-syntax-highlight" :class="$options.userColorSchemeClass">
......
...@@ -6,11 +6,18 @@ ...@@ -6,11 +6,18 @@
border-top: 1px solid $border-color; border-top: 1px solid $border-color;
} }
&.has-body {
.file-title {
box-shadow: 0 -2px 0 0 var(--white);
}
}
table.code tr:last-of-type td:last-of-type {
@include gl-rounded-bottom-right-base();
}
.file-title, .file-title,
.file-title-flex-parent { .file-title-flex-parent {
border-top-left-radius: $border-radius-default;
border-top-right-radius: $border-radius-default;
box-shadow: 0 -2px 0 0 var(--white);
cursor: pointer; cursor: pointer;
.dropdown-menu { .dropdown-menu {
...@@ -113,7 +120,6 @@ ...@@ -113,7 +120,6 @@
.diff-content { .diff-content {
background: $white; background: $white;
color: $gl-text-color; color: $gl-text-color;
border-radius: 0 0 3px 3px;
.unfold { .unfold {
cursor: pointer; cursor: pointer;
...@@ -457,8 +463,11 @@ table.code { ...@@ -457,8 +463,11 @@ table.code {
border-top: 0; border-top: 0;
} }
tr:nth-last-of-type(2).line_expansion > td { tr:nth-last-of-type(2).line_expansion,
border-bottom: 0; tr:last-of-type.line_expansion {
> td {
border-bottom: 0;
}
} }
tr.line_holder td { tr.line_holder td {
......
...@@ -11,9 +11,19 @@ ...@@ -11,9 +11,19 @@
} }
.diff-tree-list { .diff-tree-list {
// This 11px value should match the additional value found in
// /assets/stylesheets/framework/diffs.scss
// for the $mr-file-header-top SCSS variable within the
// .file-title,
// .file-title-flex-parent {
// rule.
// If they don't match, the file tree and the diff files stick
// to the top at different heights, which is a bad-looking defect
$diff-file-header-top: 11px;
$top-pos: $header-height + $mr-tabs-height + $mr-version-controls-height + $diff-file-header-top;
position: -webkit-sticky; position: -webkit-sticky;
position: sticky; position: sticky;
$top-pos: $header-height + $mr-tabs-height + $mr-version-controls-height + 15px;
top: $top-pos; top: $top-pos;
max-height: calc(100vh - #{$top-pos}); max-height: calc(100vh - #{$top-pos});
z-index: 202; z-index: 202;
......
---
title: Manually collapsed diff files are now significantly shorter and less visually
intrusive
merge_request: 43911
author:
type: changed
...@@ -697,7 +697,7 @@ describe('diffs/components/app', () => { ...@@ -697,7 +697,7 @@ describe('diffs/components/app', () => {
}); });
describe('collapsed files', () => { describe('collapsed files', () => {
it('should render the collapsed files warning if there are any collapsed files', () => { it('should render the collapsed files warning if there are any automatically collapsed files', () => {
createComponent({}, ({ state }) => { createComponent({}, ({ state }) => {
state.diffs.diffFiles = [{ viewer: { automaticallyCollapsed: true } }]; state.diffs.diffFiles = [{ viewer: { automaticallyCollapsed: true } }];
}); });
...@@ -705,16 +705,14 @@ describe('diffs/components/app', () => { ...@@ -705,16 +705,14 @@ describe('diffs/components/app', () => {
expect(getCollapsedFilesWarning(wrapper).exists()).toBe(true); expect(getCollapsedFilesWarning(wrapper).exists()).toBe(true);
}); });
it('should not render the collapsed files warning if the user has dismissed the alert already', async () => { it('should not render the collapsed files warning if there are no automatically collapsed files', () => {
createComponent({}, ({ state }) => { createComponent({}, ({ state }) => {
state.diffs.diffFiles = [{ viewer: { automaticallyCollapsed: true } }]; state.diffs.diffFiles = [
{ viewer: { automaticallyCollapsed: false, manuallyCollapsed: true } },
{ viewer: { automaticallyCollapsed: false, manuallyCollapsed: false } },
];
}); });
expect(getCollapsedFilesWarning(wrapper).exists()).toBe(true);
wrapper.vm.collapsedWarningDismissed = true;
await wrapper.vm.$nextTick();
expect(getCollapsedFilesWarning(wrapper).exists()).toBe(false); expect(getCollapsedFilesWarning(wrapper).exists()).toBe(false);
}); });
}); });
......
import { shallowMount, createLocalVue } from '@vue/test-utils'; import { shallowMount, createLocalVue } from '@vue/test-utils';
import Vuex from 'vuex'; import Vuex from 'vuex';
import { cloneDeep } from 'lodash'; import { cloneDeep } from 'lodash';
import { mockTracking, triggerEvent } from 'helpers/tracking_helper';
import DiffFileHeader from '~/diffs/components/diff_file_header.vue'; import DiffFileHeader from '~/diffs/components/diff_file_header.vue';
import ClipboardButton from '~/vue_shared/components/clipboard_button.vue'; import ClipboardButton from '~/vue_shared/components/clipboard_button.vue';
import diffDiscussionsMockData from '../mock_data/diff_discussions'; import diffDiscussionsMockData from '../mock_data/diff_discussions';
...@@ -136,9 +139,25 @@ describe('DiffFileHeader component', () => { ...@@ -136,9 +139,25 @@ describe('DiffFileHeader component', () => {
}); });
}); });
it('displays a copy to clipboard button', () => { describe('copy to clipboard', () => {
createComponent(); beforeEach(() => {
expect(wrapper.find(ClipboardButton).exists()).toBe(true); createComponent();
});
it('displays a copy to clipboard button', () => {
expect(wrapper.find(ClipboardButton).exists()).toBe(true);
});
it('triggers the copy to clipboard tracking event', () => {
const trackingSpy = mockTracking('_category_', wrapper.vm.$el, jest.spyOn);
triggerEvent('[data-testid="diff-file-copy-clipboard"]');
expect(trackingSpy).toHaveBeenCalledWith('_category_', 'click_copy_file_button', {
label: 'diff_copy_file_path_button',
property: 'diff_copy_file',
});
});
}); });
describe('for submodule', () => { describe('for submodule', () => {
......
...@@ -27,6 +27,7 @@ export default { ...@@ -27,6 +27,7 @@ export default {
name: 'text', name: 'text',
error: null, error: null,
automaticallyCollapsed: false, automaticallyCollapsed: false,
manuallyCollapsed: null,
}, },
added_lines: 2, added_lines: 2,
removed_lines: 0, removed_lines: 0,
......
...@@ -26,6 +26,7 @@ export default { ...@@ -26,6 +26,7 @@ export default {
name: 'text', name: 'text',
error: null, error: null,
automaticallyCollapsed: false, automaticallyCollapsed: false,
manuallyCollapsed: null,
}, },
added_lines: 0, added_lines: 0,
removed_lines: 0, removed_lines: 0,
......
...@@ -42,7 +42,7 @@ import { ...@@ -42,7 +42,7 @@ import {
fetchFullDiff, fetchFullDiff,
toggleFullDiff, toggleFullDiff,
switchToFullDiffFromRenamedFile, switchToFullDiffFromRenamedFile,
setFileCollapsed, setFileCollapsedByUser,
setExpandedDiffLines, setExpandedDiffLines,
setSuggestPopoverDismissed, setSuggestPopoverDismissed,
changeCurrentCommit, changeCurrentCommit,
...@@ -1216,13 +1216,18 @@ describe('DiffsStoreActions', () => { ...@@ -1216,13 +1216,18 @@ describe('DiffsStoreActions', () => {
}); });
}); });
describe('setFileCollapsed', () => { describe('setFileUserCollapsed', () => {
it('commits SET_FILE_COLLAPSED', done => { it('commits SET_FILE_COLLAPSED', done => {
testAction( testAction(
setFileCollapsed, setFileCollapsedByUser,
{ filePath: 'test', collapsed: true }, { filePath: 'test', collapsed: true },
null, null,
[{ type: types.SET_FILE_COLLAPSED, payload: { filePath: 'test', collapsed: true } }], [
{
type: types.SET_FILE_COLLAPSED,
payload: { filePath: 'test', collapsed: true, trigger: 'manual' },
},
],
[], [],
done, done,
); );
......
...@@ -49,23 +49,53 @@ describe('Diffs Module Getters', () => { ...@@ -49,23 +49,53 @@ describe('Diffs Module Getters', () => {
}); });
}); });
describe('hasCollapsedFile', () => { describe('whichCollapsedTypes', () => {
it('returns true when all files are collapsed', () => { const autoCollapsedFile = { viewer: { automaticallyCollapsed: true, manuallyCollapsed: null } };
localState.diffFiles = [ const manuallyCollapsedFile = {
{ viewer: { automaticallyCollapsed: true } }, viewer: { automaticallyCollapsed: false, manuallyCollapsed: true },
{ viewer: { automaticallyCollapsed: true } }, };
]; const openFile = { viewer: { automaticallyCollapsed: false, manuallyCollapsed: false } };
it.each`
description | value | files
${'all files are automatically collapsed'} | ${true} | ${[{ ...autoCollapsedFile }, { ...autoCollapsedFile }]}
${'all files are manually collapsed'} | ${true} | ${[{ ...manuallyCollapsedFile }, { ...manuallyCollapsedFile }]}
${'no files are collapsed in any way'} | ${false} | ${[{ ...openFile }, { ...openFile }]}
${'some files are collapsed in either way'} | ${true} | ${[{ ...manuallyCollapsedFile }, { ...autoCollapsedFile }, { ...openFile }]}
`('`any` is $value when $description', ({ value, files }) => {
localState.diffFiles = files;
const getterResult = getters.whichCollapsedTypes(localState);
expect(getterResult.any).toEqual(value);
});
it.each`
description | value | files
${'all files are automatically collapsed'} | ${true} | ${[{ ...autoCollapsedFile }, { ...autoCollapsedFile }]}
${'all files are manually collapsed'} | ${false} | ${[{ ...manuallyCollapsedFile }, { ...manuallyCollapsedFile }]}
${'no files are collapsed in any way'} | ${false} | ${[{ ...openFile }, { ...openFile }]}
${'some files are collapsed in either way'} | ${true} | ${[{ ...manuallyCollapsedFile }, { ...autoCollapsedFile }, { ...openFile }]}
`('`automatic` is $value when $description', ({ value, files }) => {
localState.diffFiles = files;
expect(getters.hasCollapsedFile(localState)).toEqual(true); const getterResult = getters.whichCollapsedTypes(localState);
expect(getterResult.automatic).toEqual(value);
}); });
it('returns true when at least one file is collapsed', () => { it.each`
localState.diffFiles = [ description | value | files
{ viewer: { automaticallyCollapsed: false } }, ${'all files are automatically collapsed'} | ${false} | ${[{ ...autoCollapsedFile }, { ...autoCollapsedFile }]}
{ viewer: { automaticallyCollapsed: true } }, ${'all files are manually collapsed'} | ${true} | ${[{ ...manuallyCollapsedFile }, { ...manuallyCollapsedFile }]}
]; ${'no files are collapsed in any way'} | ${false} | ${[{ ...openFile }, { ...openFile }]}
${'some files are collapsed in either way'} | ${true} | ${[{ ...manuallyCollapsedFile }, { ...autoCollapsedFile }, { ...openFile }]}
`('`manual` is $value when $description', ({ value, files }) => {
localState.diffFiles = files;
const getterResult = getters.whichCollapsedTypes(localState);
expect(getters.hasCollapsedFile(localState)).toEqual(true); expect(getterResult.manual).toEqual(value);
}); });
}); });
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment