Merge branch 'psi-diff-dedupe' into 'master'

Speed up expanding/collapsing diff files

See merge request gitlab-org/gitlab!24655

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

-<script>
-import { mapState, mapGetters, mapActions } from 'vuex';
-import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
-import Icon from '~/vue_shared/components/icon.vue';
-import DiffGutterAvatars from './diff_gutter_avatars.vue';
-import { LINE_POSITION_RIGHT } from '../constants';
-
-export default {
-  components: {
-    DiffGutterAvatars,
-    Icon,
-  },
-  props: {
-    line: {
-      type: Object,
-      required: true,
-    },
-    fileHash: {
-      type: String,
-      required: true,
-    },
-    contextLinesPath: {
-      type: String,
-      required: true,
-    },
-    lineNumber: {
-      type: Number,
-      required: false,
-      default: 0,
-    },
-    linePosition: {
-      type: String,
-      required: false,
-      default: '',
-    },
-    showCommentButton: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
-    isBottom: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
-    isMatchLine: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
-    isMetaLine: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
-    isContextLine: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
-    isHover: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
-  },
-  computed: {
-    ...mapState({
-      diffViewType: state => state.diffs.diffViewType,
-      diffFiles: state => state.diffs.diffFiles,
-    }),
-    ...mapGetters(['isLoggedIn']),
-    lineCode() {
-      return (
-        this.line.line_code ||
-        (this.line.left && this.line.left.line_code) ||
-        (this.line.right && this.line.right.line_code)
-      );
-    },
-    lineHref() {
-      return `#${this.line.line_code || ''}`;
-    },
-    shouldShowCommentButton() {
-      return (
-        this.isHover &&
-        !this.isMatchLine &&
-        !this.isContextLine &&
-        !this.isMetaLine &&
-        !this.hasDiscussions
-      );
-    },
-    hasDiscussions() {
-      return this.line.discussions && this.line.discussions.length > 0;
-    },
-    shouldShowAvatarsOnGutter() {
-      if (!this.line.type && this.linePosition === LINE_POSITION_RIGHT) {
-        return false;
-      }
-      return this.showCommentButton && this.hasDiscussions;
-    },
-    shouldRenderCommentButton() {
-      const isDiffHead = parseBoolean(getParameterByName('diff_head'));
-      return !isDiffHead && this.isLoggedIn && this.showCommentButton;
-    },
-  },
-  methods: {
-    ...mapActions('diffs', [
-      'loadMoreLines',
-      'showCommentForm',
-      'setHighlightedRow',
-      'toggleLineDiscussions',
-      'toggleLineDiscussionWrappers',
-    ]),
-    handleCommentButton() {
-      this.showCommentForm({ lineCode: this.line.line_code, fileHash: this.fileHash });
-    },
-  },
-};
-</script>
-
-<template>
-  <div>
-    <button
-      v-if="shouldRenderCommentButton"
-      v-show="shouldShowCommentButton"
-      type="button"
-      class="add-diff-note js-add-diff-note-button qa-diff-comment"
-      title="Add a comment to this line"
-      @click="handleCommentButton"
-    >
-      <icon :size="12" name="comment" />
-    </button>
-    <a
-      v-if="lineNumber"
-      ref="lineNumberRef"
-      :data-linenumber="lineNumber"
-      :href="lineHref"
-      @click="setHighlightedRow(lineCode)"
-    >
-    </a>
-    <diff-gutter-avatars
-      v-if="shouldShowAvatarsOnGutter"
-      :discussions="line.discussions"
-      :discussions-expanded="line.discussionsExpanded"
-      @toggleLineDiscussions="
-        toggleLineDiscussions({ lineCode, fileHash, expanded: !line.discussionsExpanded })
-      "
-    />
-  </div>
-</template>

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

 <script>
 import { mapGetters, mapActions } from 'vuex';
-import DiffLineGutterContent from './diff_line_gutter_content.vue';
+import { GlIcon } from '@gitlab/ui';
+import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
+import DiffGutterAvatars from './diff_gutter_avatars.vue';
 import {
   MATCH_LINE_TYPE,
   CONTEXT_LINE_TYPE,
+  LINE_POSITION_RIGHT,
   EMPTY_CELL_TYPE,
   OLD_LINE_TYPE,
   OLD_NO_NEW_LINE_TYPE,
   NEW_NO_NEW_LINE_TYPE,
   LINE_HOVER_CLASS_NAME,
   LINE_UNFOLD_CLASS_NAME,
-  INLINE_DIFF_VIEW_TYPE,
 } from '../constants';
 
 export default {
   components: {
-    DiffLineGutterContent,
+    DiffGutterAvatars,
+    GlIcon,
   },
   props: {
     line: {
@@ -33,12 +36,6 @@ export default {
     isHighlighted: {
       type: Boolean,
       required: true,
-      default: false,
-    },
-    diffViewType: {
-      type: String,
-      required: false,
-      default: INLINE_DIFF_VIEW_TYPE,
     },
     showCommentButton: {
       type: Boolean,
@@ -73,6 +70,38 @@ export default {
   },
   computed: {
     ...mapGetters(['isLoggedIn']),
+    lineCode() {
+      return (
+        this.line.line_code ||
+        (this.line.left && this.line.left.line_code) ||
+        (this.line.right && this.line.right.line_code)
+      );
+    },
+    lineHref() {
+      return `#${this.line.line_code || ''}`;
+    },
+    shouldShowCommentButton() {
+      return (
+        this.isHover &&
+        !this.isMatchLine &&
+        !this.isContextLine &&
+        !this.isMetaLine &&
+        !this.hasDiscussions
+      );
+    },
+    hasDiscussions() {
+      return this.line.discussions && this.line.discussions.length > 0;
+    },
+    shouldShowAvatarsOnGutter() {
+      if (!this.line.type && this.linePosition === LINE_POSITION_RIGHT) {
+        return false;
+      }
+      return this.showCommentButton && this.hasDiscussions;
+    },
+    shouldRenderCommentButton() {
+      const isDiffHead = parseBoolean(getParameterByName('diff_head'));
+      return !isDiffHead && this.isLoggedIn && this.showCommentButton;
+    },
     isMatchLine() {
       return this.line.type === MATCH_LINE_TYPE;
     },
@@ -107,24 +136,45 @@ export default {
       return this.lineType === OLD_LINE_TYPE ? this.line.old_line : this.line.new_line;
     },
   },
-  methods: mapActions('diffs', ['setHighlightedRow']),
+  methods: {
+    ...mapActions('diffs', ['showCommentForm', 'setHighlightedRow', 'toggleLineDiscussions']),
+    handleCommentButton() {
+      this.showCommentForm({ lineCode: this.line.line_code, fileHash: this.fileHash });
+    },
+  },
 };
 </script>
 
 <template>
-  <td :class="classNameMap">
-    <diff-line-gutter-content
-      :line="line"
-      :file-hash="fileHash"
-      :context-lines-path="contextLinesPath"
-      :line-position="linePosition"
-      :line-number="lineNumber"
-      :show-comment-button="showCommentButton"
-      :is-hover="isHover"
-      :is-bottom="isBottom"
-      :is-match-line="isMatchLine"
-      :is-context-line="isContentLine"
-      :is-meta-line="isMetaLine"
+  <td ref="td" :class="classNameMap">
+    <div>
+      <button
+        v-if="shouldRenderCommentButton"
+        v-show="shouldShowCommentButton"
+        ref="addDiffNoteButton"
+        type="button"
+        class="add-diff-note js-add-diff-note-button qa-diff-comment"
+        title="Add a comment to this line"
+        @click="handleCommentButton"
+      >
+        <gl-icon :size="12" name="comment" />
+      </button>
+      <a
+        v-if="lineNumber"
+        ref="lineNumberRef"
+        :data-linenumber="lineNumber"
+        :href="lineHref"
+        @click="setHighlightedRow(lineCode)"
+      >
+      </a>
+      <diff-gutter-avatars
+        v-if="shouldShowAvatarsOnGutter"
+        :discussions="line.discussions"
+        :discussions-expanded="line.discussionsExpanded"
+        @toggleLineDiscussions="
+          toggleLineDiscussions({ lineCode, fileHash, expanded: !line.discussionsExpanded })
+        "
       />
+    </div>
   </td>
 </template>

qa/qa/page/merge_request/show.rb

@@ -55,7 +55,7 @@ module QA
           element :diffs_tab
         end
 
-        view 'app/assets/javascripts/diffs/components/diff_line_gutter_content.vue' do
+        view 'app/assets/javascripts/diffs/components/diff_table_cell.vue' do
           element :diff_comment
         end
 

spec/frontend/diffs/components/diff_line_gutter_content_spec.js → spec/frontend/diffs/components/diff_table_cell_spec.js

+import { createLocalVue, shallowMount } from '@vue/test-utils';
 import Vuex from 'vuex';
-import { shallowMount, createLocalVue } from '@vue/test-utils';
-import DiffLineGutterContent from '~/diffs/components/diff_line_gutter_content.vue';
+import DiffTableCell from '~/diffs/components/diff_table_cell.vue';
 import DiffGutterAvatars from '~/diffs/components/diff_gutter_avatars.vue';
 import { LINE_POSITION_RIGHT } from '~/diffs/constants';
 import { createStore } from '~/mr_notes/stores';
@@ -17,7 +17,7 @@ const TEST_LINE_NUMBER = 1;
 const TEST_LINE_CODE = 'LC_42';
 const TEST_FILE_HASH = diffFileMockData.file_hash;
 
-describe('DiffLineGutterContent', () => {
+describe('DiffTableCell', () => {
   let wrapper;
   let line;
   let store;
@@ -49,22 +49,40 @@ describe('DiffLineGutterContent', () => {
       value,
     });
   };
+
   const createComponent = (props = {}) => {
-    wrapper = shallowMount(DiffLineGutterContent, {
+    wrapper = shallowMount(DiffTableCell, {
       localVue,
       store,
       propsData: {
         line,
         fileHash: TEST_FILE_HASH,
         contextLinesPath: '/context/lines/path',
+        isHighlighted: false,
         ...props,
       },
     });
   };
-  const findNoteButton = () => wrapper.find('.js-add-diff-note-button');
+
+  const findTd = () => wrapper.find({ ref: 'td' });
+  const findNoteButton = () => wrapper.find({ ref: 'addDiffNoteButton' });
   const findLineNumber = () => wrapper.find({ ref: 'lineNumberRef' });
   const findAvatars = () => wrapper.find(DiffGutterAvatars);
 
+  describe('td', () => {
+    it('highlights when isHighlighted true', () => {
+      createComponent({ isHighlighted: true });
+
+      expect(findTd().classes()).toContain('hll');
+    });
+
+    it('does not highlight when isHighlighted false', () => {
+      createComponent({ isHighlighted: false });
+
+      expect(findTd().classes()).not.toContain('hll');
+    });
+  });
+
   describe('comment button', () => {
     it.each`
       showCommentButton | userData     | query                | expectation
@@ -87,9 +105,9 @@ describe('DiffLineGutterContent', () => {
       isHover  | otherProps                                      | discussions | expectation
       ${true}  | ${{}}                                           | ${[]}       | ${true}
       ${false} | ${{}}                                           | ${[]}       | ${false}
-      ${true}  | ${{ isMatchLine: true }}   | ${[]}       | ${false}
-      ${true}  | ${{ isContextLine: true }} | ${[]}       | ${false}
-      ${true}  | ${{ isMetaLine: true }}    | ${[]}       | ${false}
+      ${true}  | ${{ line: { ...line, type: 'match' } }}         | ${[]}       | ${false}
+      ${true}  | ${{ line: { ...line, type: 'context' } }}       | ${[]}       | ${false}
+      ${true}  | ${{ line: { ...line, type: 'old-nonewline' } }} | ${[]}       | ${false}
       ${true}  | ${{}}                                           | ${[{}]}     | ${false}
     `(
       'visible is $expectation - with isHover ($isHover), discussions ($discussions), otherProps ($otherProps)',
@@ -109,7 +127,7 @@ describe('DiffLineGutterContent', () => {
   describe('line number', () => {
     describe('without lineNumber prop', () => {
       it('does not render', () => {
-        createComponent();
+        createComponent({ lineType: 'old' });
 
         expect(findLineNumber().exists()).toBe(false);
       });

spec/javascripts/diffs/components/diff_table_cell_spec.js

-import Vue from 'vue';
-import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
-import { createStore } from '~/mr_notes/stores';
-import DiffTableCell from '~/diffs/components/diff_table_cell.vue';
-import diffFileMockData from '../mock_data/diff_file';
-
-describe('DiffTableCell', () => {
-  const createComponent = options =>
-    createComponentWithStore(Vue.extend(DiffTableCell), createStore(), {
-      line: diffFileMockData.highlighted_diff_lines[0],
-      fileHash: diffFileMockData.file_hash,
-      contextLinesPath: 'contextLinesPath',
-      ...options,
-    }).$mount();
-
-  it('does not highlight row when isHighlighted prop is false', done => {
-    const vm = createComponent({ isHighlighted: false });
-
-    vm.$nextTick()
-      .then(() => {
-        expect(vm.$el.classList).not.toContain('hll');
-      })
-      .then(done)
-      .catch(done.fail);
-  });
-
-  it('highlights row when isHighlighted prop is true', done => {
-    const vm = createComponent({ isHighlighted: true });
-
-    vm.$nextTick()
-      .then(() => {
-        expect(vm.$el.classList).toContain('hll');
-      })
-      .then(done)
-      .catch(done.fail);
-  });
-});