Merge branch 'mr-file-tree-blob-truncate-improvements' into 'master'

Add headers to files in the tree list on merge requests

Closes #54807

See merge request gitlab-org/gitlab-ce!23954

app/assets/javascripts/diffs/components/tree_list.vue

@@ -34,14 +34,18 @@ export default {
 
       if (search === '') return this.renderTreeList ? this.tree : this.allBlobs;
 
-      return this.allBlobs.filter(f => f.path.toLowerCase().indexOf(search) >= 0);
-    },
-    rowDisplayTextKey() {
-      if (this.renderTreeList && this.search.trim() === '') {
-        return 'name';
-      }
+      return this.allBlobs.reduce((acc, folder) => {
+        const tree = folder.tree.filter(f => f.path.toLowerCase().indexOf(search) >= 0);
 
-      return 'path';
+        if (tree.length) {
+          return acc.concat({
+            ...folder,
+            tree,
+          });
+        }
+
+        return acc;
+      }, []);
     },
   },
   methods: {
@@ -119,7 +123,7 @@ export default {
         </button>
       </div>
     </div>
-    <div class="tree-list-scroll">
+    <div :class="{ 'pt-0 tree-list-blobs': !renderTreeList }" class="tree-list-scroll">
       <template v-if="filteredTreeList.length">
         <file-row
           v-for="file in filteredTreeList"
@@ -129,8 +133,6 @@ export default {
           :hide-extra-on-tree="true"
           :extra-component="$options.FileRowStats"
           :show-changed-icon="true"
-          :display-text-key="rowDisplayTextKey"
-          :should-truncate-start="true"
           @toggleTreeOpen="toggleTreeOpen"
           @clickFile="scrollToFile"
         />
@@ -148,3 +150,9 @@ export default {
     </div>
   </div>
 </template>
+
+<style>
+.tree-list-blobs .file-row-name {
+  margin-left: 12px;
+}
+</style>

app/assets/javascripts/diffs/store/getters.js

@@ -74,7 +74,24 @@ export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) =
 export const getDiffFileByHash = state => fileHash =>
   state.diffFiles.find(file => file.file_hash === fileHash);
 
-export const allBlobs = state => Object.values(state.treeEntries).filter(f => f.type === 'blob');
+export const allBlobs = state =>
+  Object.values(state.treeEntries)
+    .filter(f => f.type === 'blob')
+    .reduce((acc, file) => {
+      const { parentPath } = file;
+
+      if (parentPath && !acc.some(f => f.path === parentPath)) {
+        acc.push({
+          path: parentPath,
+          isHeader: true,
+          tree: [],
+        });
+      }
+
+      acc.find(f => f.path === parentPath).tree.push(file);
+
+      return acc;
+    }, []);
 
 export const diffFilesLength = state => state.diffFiles.length;
 

app/assets/javascripts/diffs/store/utils.js

@@ -318,6 +318,7 @@ export const generateTreeList = files =>
               fileHash: file.file_hash,
               addedLines: file.added_lines,
               removedLines: file.removed_lines,
+              parentPath: parent ? `${parent.path}/` : '/',
             });
           } else {
             Object.assign(entry, {

app/assets/javascripts/lib/utils/text_utility.js

@@ -72,6 +72,29 @@ export const truncate = (string, maxLength) => `${string.substr(0, maxLength - 3
  */
 export const truncateSha = sha => sha.substr(0, 8);
 
+const ELLIPSIS_CHAR = '…';
+export const truncatePathMiddleToLength = (text, maxWidth) => {
+  let returnText = text;
+  let ellipsisCount = 0;
+
+  while (returnText.length >= maxWidth) {
+    const textSplit = returnText.split('/').filter(s => s !== ELLIPSIS_CHAR);
+    const middleIndex = Math.floor(textSplit.length / 2);
+
+    returnText = textSplit
+      .slice(0, middleIndex)
+      .concat(
+        new Array(ellipsisCount + 1).fill().map(() => ELLIPSIS_CHAR),
+        textSplit.slice(middleIndex + 1),
+      )
+      .join('/');
+
+    ellipsisCount += 1;
+  }
+
+  return returnText;
+};
+
 /**
  * Capitalizes first character
  *

app/assets/javascripts/vue_shared/components/file_row.vue

 <script>
 import Icon from '~/vue_shared/components/icon.vue';
+import FileHeader from '~/vue_shared/components/file_row_header.vue';
 import FileIcon from '~/vue_shared/components/file_icon.vue';
 import ChangedFileIcon from '~/vue_shared/components/changed_file_icon.vue';
 
 export default {
   name: 'FileRow',
   components: {
+    FileHeader,
     FileIcon,
     Icon,
     ChangedFileIcon,
@@ -34,21 +36,10 @@ export default {
       required: false,
       default: false,
     },
-    displayTextKey: {
-      type: String,
-      required: false,
-      default: 'name',
-    },
-    shouldTruncateStart: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
   },
   data() {
     return {
       mouseOver: false,
-      truncateStart: 0,
     };
   },
   computed: {
@@ -60,7 +51,7 @@ export default {
     },
     levelIndentation() {
       return {
-        marginLeft: `${this.level * 16}px`,
+        marginLeft: this.level ? `${this.level * 16}px` : null,
       };
     },
     fileClass() {
@@ -71,14 +62,8 @@ export default {
         'is-open': this.file.opened,
       };
     },
-    outputText() {
-      const text = this.file[this.displayTextKey];
-
-      if (this.truncateStart === 0) {
-        return text;
-      }
-
-      return `...${text.substring(this.truncateStart, text.length)}`;
+    childFilesLevel() {
+      return this.file.isHeader ? 0 : this.level + 1;
     },
   },
   watch: {
@@ -92,15 +77,6 @@ export default {
     if (this.hasPathAtCurrentRoute()) {
       this.scrollIntoView(true);
     }
-
-    if (this.shouldTruncateStart) {
-      const { scrollWidth, offsetWidth } = this.$refs.textOutput;
-      const textOverflow = scrollWidth - offsetWidth;
-
-      if (textOverflow > 0) {
-        this.truncateStart = Math.ceil(textOverflow / 5) + 3;
-      }
-    }
   },
   methods: {
     toggleTreeOpen(path) {
@@ -156,7 +132,9 @@ export default {
 
 <template>
   <div>
+    <file-header v-if="file.isHeader" :path="file.path" />
     <div
+      v-else
       :class="fileClass"
       class="file-row"
       role="button"
@@ -175,7 +153,7 @@ export default {
             :size="16"
           />
           <changed-file-icon v-else :file="file" :size="16" class="append-right-5" />
-          {{ outputText }}
+          {{ file.name }}
         </span>
         <component
           :is="extraComponent"
@@ -185,17 +163,15 @@ export default {
         />
       </div>
     </div>
-    <template v-if="file.opened">
+    <template v-if="file.opened || file.isHeader">
       <file-row
         v-for="childFile in file.tree"
         :key="childFile.key"
         :file="childFile"
-        :level="level + 1"
+        :level="childFilesLevel"
         :hide-extra-on-tree="hideExtraOnTree"
         :extra-component="extraComponent"
         :show-changed-icon="showChangedIcon"
-        :display-text-key="displayTextKey"
-        :should-truncate-start="shouldTruncateStart"
         @toggleTreeOpen="toggleTreeOpen"
         @clickFile="clickedFile"
       />

app/assets/javascripts/vue_shared/components/file_row_header.vue

+<script>
+import { truncatePathMiddleToLength } from '~/lib/utils/text_utility';
+
+const MAX_PATH_LENGTH = 40;
+
+export default {
+  props: {
+    path: {
+      type: String,
+      required: true,
+    },
+  },
+  computed: {
+    truncatedPath() {
+      return truncatePathMiddleToLength(this.path, MAX_PATH_LENGTH);
+    },
+  },
+};
+</script>
+
+<template>
+  <div class="file-row-header bg-white sticky-top p-2 js-file-row-header">
+    <span class="bold">{{ truncatedPath }}</span>
+  </div>
+</template>

changelogs/unreleased/mr-file-tree-blob-truncate-improvements.yml

+---
+title: Add folder header to files in merge request tree list
+merge_request:
+author:
+type: changed

spec/frontend/vue_shared/components/__snapshots__/file_row_header_spec.js.snap

+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`File row header component adds multiple ellipsises after 40 characters 1`] = `
+<div
+  class="file-row-header bg-white sticky-top p-2 js-file-row-header"
+>
+  <span
+    class="bold"
+  >
+    app/assets/javascripts/…/…/diffs/notes
+  </span>
+</div>
+`;
+
+exports[`File row header component renders file path 1`] = `
+<div
+  class="file-row-header bg-white sticky-top p-2 js-file-row-header"
+>
+  <span
+    class="bold"
+  >
+    app/assets
+  </span>
+</div>
+`;
+
+exports[`File row header component trucates path after 40 characters 1`] = `
+<div
+  class="file-row-header bg-white sticky-top p-2 js-file-row-header"
+>
+  <span
+    class="bold"
+  >
+    app/assets/javascripts/merge_requests
+  </span>
+</div>
+`;

spec/frontend/vue_shared/components/file_row_header_spec.js

+import { shallowMount } from '@vue/test-utils';
+import FileRowHeader from '~/vue_shared/components/file_row_header.vue';
+
+describe('File row header component', () => {
+  let vm;
+
+  function createComponent(path) {
+    vm = shallowMount(FileRowHeader, {
+      propsData: {
+        path,
+      },
+    });
+  }
+
+  afterEach(() => {
+    vm.destroy();
+  });
+
+  it('renders file path', () => {
+    createComponent('app/assets');
+
+    expect(vm.element).toMatchSnapshot();
+  });
+
+  it('trucates path after 40 characters', () => {
+    createComponent('app/assets/javascripts/merge_requests');
+
+    expect(vm.element).toMatchSnapshot();
+  });
+
+  it('adds multiple ellipsises after 40 characters', () => {
+    createComponent('app/assets/javascripts/merge_requests/widget/diffs/notes');
+
+    expect(vm.element).toMatchSnapshot();
+  });
+});

spec/javascripts/diffs/components/tree_list_spec.js

@@ -26,6 +26,8 @@ describe('Diffs tree list component', () => {
     store.state.diffs.removedLines = 20;
     store.state.diffs.diffFiles.push('test');
 
+    localStorage.removeItem('mr_diff_tree_list');
+
     vm = mountComponentWithStore(Component, { store });
   });
 
@@ -57,6 +59,7 @@ describe('Diffs tree list component', () => {
           removedLines: 0,
           tempFile: true,
           type: 'blob',
+          parentPath: 'app',
         },
         app: {
           key: 'app',
@@ -121,7 +124,7 @@ describe('Diffs tree list component', () => {
       vm.renderTreeList = false;
 
       vm.$nextTick(() => {
-        expect(vm.$el.querySelector('.file-row').textContent).toContain('app/index.js');
+        expect(vm.$el.querySelector('.file-row').textContent).toContain('index.js');
 
         done();
       });

spec/javascripts/diffs/store/getters_spec.js

@@ -230,15 +230,30 @@ describe('Diffs Module Getters', () => {
       localState.treeEntries = {
         file: {
           type: 'blob',
+          path: 'file',
+          parentPath: '/',
+          tree: [],
         },
         tree: {
           type: 'tree',
+          path: 'tree',
+          parentPath: '/',
+          tree: [],
         },
       };
 
       expect(getters.allBlobs(localState)).toEqual([
         {
-          type: 'blob',
+          isHeader: true,
+          path: '/',
+          tree: [
+            {
+              parentPath: '/',
+              path: 'file',
+              tree: [],
+              type: 'blob',
+            },
+          ],
         },
       ]);
     });

spec/javascripts/diffs/store/utils_spec.js

@@ -502,6 +502,7 @@ describe('DiffsStoreUtils', () => {
               fileHash: 'test',
               key: 'app/index.js',
               name: 'index.js',
+              parentPath: 'app/',
               path: 'app/index.js',
               removedLines: 10,
               tempFile: false,
@@ -522,6 +523,7 @@ describe('DiffsStoreUtils', () => {
                   fileHash: 'test',
                   key: 'app/test/index.js',
                   name: 'index.js',
+                  parentPath: 'app/test/',
                   path: 'app/test/index.js',
                   removedLines: 0,
                   tempFile: true,
@@ -535,6 +537,7 @@ describe('DiffsStoreUtils', () => {
                   fileHash: 'test',
                   key: 'app/test/filepathneedstruncating.js',
                   name: 'filepathneedstruncating.js',
+                  parentPath: 'app/test/',
                   path: 'app/test/filepathneedstruncating.js',
                   removedLines: 0,
                   tempFile: true,
@@ -548,6 +551,7 @@ describe('DiffsStoreUtils', () => {
         },
         {
           key: 'package.json',
+          parentPath: '/',
           path: 'package.json',
           name: 'package.json',
           type: 'blob',

spec/javascripts/lib/utils/text_utility_spec.js

@@ -135,4 +135,20 @@ describe('text_utility', () => {
       expect(textUtils.getFirstCharacterCapitalized(null)).toEqual('');
     });
   });
+
+  describe('truncatePathMiddleToLength', () => {
+    it('does not truncate text', () => {
+      expect(textUtils.truncatePathMiddleToLength('app/test', 50)).toEqual('app/test');
+    });
+
+    it('truncates middle of the path', () => {
+      expect(textUtils.truncatePathMiddleToLength('app/test/diff', 13)).toEqual('app/…/diff');
+    });
+
+    it('truncates multiple times in the middle of the path', () => {
+      expect(textUtils.truncatePathMiddleToLength('app/test/merge_request/diff', 13)).toEqual(
+        'app/…/…/diff',
+      );
+    });
+  });
 });

spec/javascripts/vue_shared/components/file_row_spec.js

@@ -3,7 +3,7 @@ import FileRow from '~/vue_shared/components/file_row.vue';
 import { file } from 'spec/ide/helpers';
 import mountComponent from '../../helpers/vue_mount_component_helper';
 
-describe('RepoFile', () => {
+describe('File row component', () => {
   let vm;
 
   function createComponent(propsData) {
@@ -72,39 +72,16 @@ describe('RepoFile', () => {
     expect(vm.$el.querySelector('.file-row-name').style.marginLeft).toBe('32px');
   });
 
-  describe('outputText', () => {
-    beforeEach(done => {
-      createComponent({
-        file: {
-          ...file(),
-          path: 'app/assets/index.js',
-        },
-        level: 0,
-      });
-
-      vm.displayTextKey = 'path';
-
-      vm.$nextTick(done);
-    });
-
-    it('returns text if truncateStart is 0', done => {
-      vm.truncateStart = 0;
-
-      vm.$nextTick(() => {
-        expect(vm.outputText).toBe('app/assets/index.js');
-
-        done();
-      });
+  it('renders header for file', () => {
+    createComponent({
+      file: {
+        isHeader: true,
+        path: 'app/assets',
+        tree: [],
+      },
+      level: 0,
     });
 
-    it('returns text truncated at start', done => {
-      vm.truncateStart = 5;
-
-      vm.$nextTick(() => {
-        expect(vm.outputText).toBe('...ssets/index.js');
-
-        done();
-      });
-    });
+    expect(vm.$el.querySelector('.js-file-row-header')).not.toBe(null);
   });
 });