Merge branch '48496-merge-request-refactor-does-not-highlight-selected-line' into 'master'

Resolve "Merge request refactor does not highlight selected line"

Closes #48496

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

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

@@ -102,6 +102,12 @@ export default {
     if (this.shouldShow) {
       this.fetchData();
     }
+
+    const id = window && window.location && window.location.hash;
+
+    if (id) {
+      this.setHighlightedRow(id.slice(1));
+    }
   },
   created() {
     this.adjustView();
@@ -114,6 +120,7 @@ export default {
       'fetchDiffFiles',
       'startRenderDiffsQueue',
       'assignDiscussionsToDiff',
+      'setHighlightedRow',
     ]),
     fetchData() {
       this.fetchDiffFiles()

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

@@ -72,6 +72,13 @@ export default {
       diffFiles: state => state.diffs.diffFiles,
     }),
     ...mapGetters(['isLoggedIn']),
+    lineCode() {
+      return (
+        this.line.line_code ||
+        (this.line.left && this.line.line.left.line_code) ||
+        (this.line.right && this.line.right.line_code)
+      );
+    },
     lineHref() {
       return `#${this.line.line_code || ''}`;
     },
@@ -97,7 +104,7 @@ export default {
     },
   },
   methods: {
-    ...mapActions('diffs', ['loadMoreLines', 'showCommentForm']),
+    ...mapActions('diffs', ['loadMoreLines', 'showCommentForm', 'setHighlightedRow']),
     handleCommentButton() {
       this.showCommentForm({ lineCode: this.line.line_code, fileHash: this.fileHash });
     },
@@ -168,7 +175,13 @@ export default {
       >
         <icon :size="12" name="comment" />
       </button>
-      <a v-if="lineNumber" :data-linenumber="lineNumber" :href="lineHref"> </a>
+      <a
+        v-if="lineNumber"
+        :data-linenumber="lineNumber"
+        :href="lineHref"
+        @click="setHighlightedRow(lineCode);"
+      >
+      </a>
       <diff-gutter-avatars v-if="shouldShowAvatarsOnGutter" :discussions="line.discussions" />
     </template>
   </div>

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

 <script>
-import { mapGetters } from 'vuex';
+import { mapGetters, mapActions } from 'vuex';
 import DiffLineGutterContent from './diff_line_gutter_content.vue';
 import {
   MATCH_LINE_TYPE,
@@ -30,6 +30,11 @@ export default {
       type: String,
       required: true,
     },
+    isHighlighted: {
+      type: Boolean,
+      required: true,
+      default: false,
+    },
     diffViewType: {
       type: String,
       required: false,
@@ -85,6 +90,7 @@ export default {
       const { type } = this.line;
 
       return {
+        hll: this.isHighlighted,
         [type]: type,
         [LINE_UNFOLD_CLASS_NAME]: this.isMatchLine,
         [LINE_HOVER_CLASS_NAME]:
@@ -99,6 +105,7 @@ export default {
       return this.lineType === OLD_LINE_TYPE ? this.line.old_line : this.line.new_line;
     },
   },
+  methods: mapActions('diffs', ['setHighlightedRow']),
 };
 </script>
 

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

 <script>
-import { mapGetters, mapActions } from 'vuex';
+import { mapGetters, mapActions, mapState } from 'vuex';
 import DiffTableCell from './diff_table_cell.vue';
 import {
   NEW_LINE_TYPE,
@@ -40,6 +40,11 @@ export default {
     };
   },
   computed: {
+    ...mapState({
+      isHighlighted(state) {
+        return this.line.line_code !== null && this.line.line_code === state.diffs.highlightedRow;
+      },
+    }),
     ...mapGetters('diffs', ['isInlineView']),
     isContextLine() {
       return this.line.type === CONTEXT_LINE_TYPE;
@@ -91,6 +96,7 @@ export default {
       :is-bottom="isBottom"
       :is-hover="isHover"
       :show-comment-button="true"
+      :is-highlighted="isHighlighted"
       class="diff-line-num old_line"
     />
     <diff-table-cell
@@ -100,8 +106,18 @@ export default {
       :line-type="newLineType"
       :is-bottom="isBottom"
       :is-hover="isHover"
+      :is-highlighted="isHighlighted"
       class="diff-line-num new_line qa-new-diff-line"
     />
-    <td :class="line.type" class="line_content" v-html="line.rich_text"></td>
+    <td
+      :class="[
+        line.type,
+        {
+          hll: isHighlighted,
+        },
+      ]"
+      class="line_content"
+      v-html="line.rich_text"
+    ></td>
   </tr>
 </template>

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

 <script>
-import { mapActions } from 'vuex';
+import { mapActions, mapState } from 'vuex';
 import $ from 'jquery';
 import DiffTableCell from './diff_table_cell.vue';
 import {
@@ -43,6 +43,15 @@ export default {
     };
   },
   computed: {
+    ...mapState({
+      isHighlighted(state) {
+        const lineCode =
+          (this.line.left && this.line.left.line_code) ||
+          (this.line.right && this.line.right.line_code);
+
+        return lineCode ? lineCode === state.diffs.highlightedRow : false;
+      },
+    }),
     isContextLine() {
       return this.line.left && this.line.left.type === CONTEXT_LINE_TYPE;
     },
@@ -57,7 +66,14 @@ export default {
         return OLD_NO_NEW_LINE_TYPE;
       }
 
-      return this.line.left ? this.line.left.type : EMPTY_CELL_TYPE;
+      const lineTypeClass = this.line.left ? this.line.left.type : EMPTY_CELL_TYPE;
+
+      return [
+        lineTypeClass,
+        {
+          hll: this.isHighlighted,
+        },
+      ];
     },
   },
   created() {
@@ -114,6 +130,7 @@ export default {
         :line-type="oldLineType"
         :is-bottom="isBottom"
         :is-hover="isLeftHover"
+        :is-highlighted="isHighlighted"
         :show-comment-button="true"
         :diff-view-type="parallelDiffViewType"
         line-position="left"
@@ -139,6 +156,7 @@ export default {
         :line-type="newLineType"
         :is-bottom="isBottom"
         :is-hover="isRightHover"
+        :is-highlighted="isHighlighted"
         :show-comment-button="true"
         :diff-view-type="parallelDiffViewType"
         line-position="right"
@@ -146,7 +164,12 @@ export default {
       />
       <td
         :id="line.right.line_code"
-        :class="line.right.type"
+        :class="[
+          line.right.type,
+          {
+            hll: isHighlighted,
+          },
+        ]"
         class="line_content parallel right-side"
         @mousedown.native="handleParallelLineMouseDown"
         v-html="line.right.rich_text"

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

@@ -33,6 +33,10 @@ export const fetchDiffFiles = ({ state, commit }) => {
     .then(handleLocationHash);
 };
 
+export const setHighlightedRow = ({ commit }, lineCode) => {
+  commit(types.SET_HIGHLIGHTED_ROW, lineCode);
+};
+
 // This is adding line discussions to the actual lines in the diff tree
 // once for parallel and once for inline mode
 export const assignDiscussionsToDiff = (
@@ -127,7 +131,7 @@ export const loadMoreLines = ({ commit }, options) => {
 export const scrollToLineIfNeededInline = (_, line) => {
   const hash = getLocationHash();
 
-  if (hash && line.lineCode === hash) {
+  if (hash && line.line_code === hash) {
     handleLocationHash();
   }
 };
@@ -137,7 +141,7 @@ export const scrollToLineIfNeededParallel = (_, line) => {
 
   if (
     hash &&
-    ((line.left && line.left.lineCode === hash) || (line.right && line.right.lineCode === hash))
+    ((line.left && line.left.line_code === hash) || (line.right && line.right.line_code === hash))
   ) {
     handleLocationHash();
   }

app/assets/javascripts/diffs/store/modules/diff_state.js

@@ -26,4 +26,5 @@ export default () => ({
   currentDiffFileId: '',
   projectPath: '',
   commentForms: [],
+  highlightedRow: null,
 });

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

@@ -17,3 +17,4 @@ export const UPDATE_CURRENT_DIFF_FILE_ID = 'UPDATE_CURRENT_DIFF_FILE_ID';
 export const OPEN_DIFF_FILE_COMMENT_FORM = 'OPEN_DIFF_FILE_COMMENT_FORM';
 export const UPDATE_DIFF_FILE_COMMENT_FORM = 'UPDATE_DIFF_FILE_COMMENT_FORM';
 export const CLOSE_DIFF_FILE_COMMENT_FORM = 'CLOSE_DIFF_FILE_COMMENT_FORM';
+export const SET_HIGHLIGHTED_ROW = 'SET_HIGHLIGHTED_ROW';

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

@@ -241,4 +241,7 @@ export default {
   [types.CLOSE_DIFF_FILE_COMMENT_FORM](state, fileHash) {
     state.commentForms = state.commentForms.filter(form => form.fileHash !== fileHash);
   },
+  [types.SET_HIGHLIGHTED_ROW](state, lineCode) {
+    state.highlightedRow = lineCode;
+  },
 };

changelogs/unreleased/48496-merge-request-refactor-does-not-highlight-selected-line.yml

+---
+title: When user clicks linenumber in MR changes, highlight that line
+merge_request:
+author:
+type: fixed

spec/javascripts/diffs/components/app_spec.js

@@ -13,6 +13,8 @@ describe('diffs/components/app', () => {
   beforeEach(() => {
     // setup globals (needed for component to mount :/)
     window.mrTabs = jasmine.createSpyObj('mrTabs', ['resetViewContainer']);
+    window.mrTabs.expandViewContainer = jasmine.createSpy();
+    window.location.hash = 'ABC_123';
 
     // setup component
     const store = createDiffsStore();
@@ -39,4 +41,15 @@ describe('diffs/components/app', () => {
   it('does not show commit info', () => {
     expect(vm.$el).not.toContainElement('.blob-commit-info');
   });
+
+  it('sets highlighted row if hash exists in location object', done => {
+    vm.$props.shouldShow = true;
+
+    vm.$nextTick()
+      .then(() => {
+        expect(vm.$store.state.diffs.highlightedRow).toBe('ABC_123');
+      })
+      .then(done)
+      .catch(done.fail);
+  });
 });

spec/javascripts/diffs/components/diff_table_cell_spec.js

+import Vue from 'vue';
+import store from '~/mr_notes/stores';
+import DiffTableCell from '~/diffs/components/diff_table_cell.vue';
+import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
+import diffFileMockData from '../mock_data/diff_file';
+
+describe('DiffTableCell', () => {
+  const createComponent = options =>
+    createComponentWithStore(Vue.extend(DiffTableCell), store, {
+      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);
+  });
+});

spec/javascripts/diffs/components/inline_diff_table_row_spec.js

+import Vue from 'vue';
+import store from '~/mr_notes/stores';
+import InlineDiffTableRow from '~/diffs/components/inline_diff_table_row.vue';
+import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
+import diffFileMockData from '../mock_data/diff_file';
+
+describe('InlineDiffTableRow', () => {
+  let vm;
+  const thisLine = diffFileMockData.highlighted_diff_lines[0];
+
+  beforeEach(() => {
+    vm = createComponentWithStore(Vue.extend(InlineDiffTableRow), store, {
+      line: thisLine,
+      fileHash: diffFileMockData.file_hash,
+      contextLinesPath: 'contextLinesPath',
+      isHighlighted: false,
+    }).$mount();
+  });
+
+  it('does not add hll class to line content when line does not match highlighted row', done => {
+    vm.$nextTick()
+      .then(() => {
+        expect(vm.$el.querySelector('.line_content').classList).not.toContain('hll');
+      })
+      .then(done)
+      .catch(done.fail);
+  });
+
+  it('adds hll class to lineContent when line is the highlighted row', done => {
+    vm.$nextTick()
+      .then(() => {
+        vm.$store.state.diffs.highlightedRow = thisLine.line_code;
+
+        return vm.$nextTick();
+      })
+      .then(() => {
+        expect(vm.$el.querySelector('.line_content').classList).toContain('hll');
+      })
+      .then(done)
+      .catch(done.fail);
+  });
+});

spec/javascripts/diffs/components/parallel_diff_table_row_spec.js

+import Vue from 'vue';
+import { createStore } from '~/mr_notes/stores';
+import ParallelDiffTableRow from '~/diffs/components/parallel_diff_table_row.vue';
+import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
+import diffFileMockData from '../mock_data/diff_file';
+
+describe('ParallelDiffTableRow', () => {
+  describe('when one side is empty', () => {
+    let vm;
+    const thisLine = diffFileMockData.parallel_diff_lines[0];
+    const rightLine = diffFileMockData.parallel_diff_lines[0].right;
+
+    beforeEach(() => {
+      vm = createComponentWithStore(Vue.extend(ParallelDiffTableRow), createStore(), {
+        line: thisLine,
+        fileHash: diffFileMockData.file_hash,
+        contextLinesPath: 'contextLinesPath',
+        isHighlighted: false,
+      }).$mount();
+    });
+
+    it('does not highlight non empty line content when line does not match highlighted row', done => {
+      vm.$nextTick()
+        .then(() => {
+          expect(vm.$el.querySelector('.line_content.right-side').classList).not.toContain('hll');
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('highlights nonempty line content when line is the highlighted row', done => {
+      vm.$nextTick()
+        .then(() => {
+          vm.$store.state.diffs.highlightedRow = rightLine.line_code;
+
+          return vm.$nextTick();
+        })
+        .then(() => {
+          expect(vm.$el.querySelector('.line_content.right-side').classList).toContain('hll');
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
+
+  describe('when both sides have content', () => {
+    let vm;
+    const thisLine = diffFileMockData.parallel_diff_lines[2];
+    const rightLine = diffFileMockData.parallel_diff_lines[2].right;
+
+    beforeEach(() => {
+      vm = createComponentWithStore(Vue.extend(ParallelDiffTableRow), createStore(), {
+        line: thisLine,
+        fileHash: diffFileMockData.file_hash,
+        contextLinesPath: 'contextLinesPath',
+        isHighlighted: false,
+      }).$mount();
+    });
+
+    it('does not highlight  either line when line does not match highlighted row', done => {
+      vm.$nextTick()
+        .then(() => {
+          expect(vm.$el.querySelector('.line_content.right-side').classList).not.toContain('hll');
+          expect(vm.$el.querySelector('.line_content.left-side').classList).not.toContain('hll');
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('adds hll class to lineContent when line is the highlighted row', done => {
+      vm.$nextTick()
+        .then(() => {
+          vm.$store.state.diffs.highlightedRow = rightLine.line_code;
+
+          return vm.$nextTick();
+        })
+        .then(() => {
+          expect(vm.$el.querySelector('.line_content.right-side').classList).toContain('hll');
+          expect(vm.$el.querySelector('.line_content.left-side').classList).toContain('hll');
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
+});

spec/javascripts/diffs/store/actions_spec.js

@@ -22,6 +22,7 @@ import actions, {
   expandAllFiles,
   toggleFileDiscussions,
   saveDiffDiscussion,
+  setHighlightedRow,
   toggleTreeOpen,
   scrollToFile,
   toggleShowTreeList,
@@ -92,6 +93,14 @@ describe('DiffsStoreActions', () => {
     });
   });
 
+  describe('setHighlightedRow', () => {
+    it('should set lineHash and fileHash of highlightedRow', () => {
+      testAction(setHighlightedRow, 'ABC_123', {}, [
+        { type: types.SET_HIGHLIGHTED_ROW, payload: 'ABC_123' },
+      ]);
+    });
+  });
+
   describe('assignDiscussionsToDiff', () => {
     it('should merge discussions into diffs', done => {
       const state = {
@@ -469,7 +478,7 @@ describe('DiffsStoreActions', () => {
 
   describe('scrollToLineIfNeededInline', () => {
     const lineMock = {
-      lineCode: 'ABC_123',
+      line_code: 'ABC_123',
     };
 
     it('should not call handleLocationHash when there is not hash', () => {
@@ -520,7 +529,7 @@ describe('DiffsStoreActions', () => {
     const lineMock = {
       left: null,
       right: {
-        lineCode: 'ABC_123',
+        line_code: 'ABC_123',
       },
     };
 

spec/javascripts/diffs/store/mutations_spec.js

@@ -360,6 +360,16 @@ describe('DiffsStoreMutations', () => {
     });
   });
 
+  describe('Set highlighted row', () => {
+    it('sets highlighted row', () => {
+      const state = createState();
+
+      mutations[types.SET_HIGHLIGHTED_ROW](state, 'ABC_123');
+
+      expect(state.highlightedRow).toBe('ABC_123');
+    });
+  });
+
   describe('TOGGLE_LINE_HAS_FORM', () => {
     it('sets hasForm on lines', () => {
       const file = {