diff options
author | Phil Hughes <me@iamphill.com> | 2019-02-27 12:12:13 +0300 |
---|---|---|
committer | Phil Hughes <me@iamphill.com> | 2019-02-27 12:12:13 +0300 |
commit | 5a38068a0e8fbc149a0a586d5825d21f9cb124f4 (patch) | |
tree | 994b53c9de178e06dfca697044c231651749a369 | |
parent | d40a3809fd387f8dc9a28218a004260b600a1412 (diff) | |
parent | 0aff8e27530d391e4c819838b46d77f6231ecf70 (diff) |
Merge branch '48798-keybinding-mr-diff' into 'master'
Resolve "Add keybinding for next/previous file in merge request diff"
See merge request gitlab-org/gitlab-ce!25355
-rw-r--r-- | app/assets/javascripts/diffs/components/app.vue | 33 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/store/actions.js | 4 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/store/getters.js | 7 | ||||
-rw-r--r-- | app/views/help/_shortcuts.html.haml | 12 | ||||
-rw-r--r-- | changelogs/unreleased/48798-keybinding-mr-diff.yml | 5 | ||||
-rw-r--r-- | doc/workflow/shortcuts.md | 2 | ||||
-rw-r--r-- | spec/javascripts/diffs/components/app_spec.js | 224 | ||||
-rw-r--r-- | spec/javascripts/diffs/store/actions_spec.js | 19 | ||||
-rw-r--r-- | spec/javascripts/diffs/store/getters_spec.js | 20 |
9 files changed, 287 insertions, 39 deletions
diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 8f47931d14a..1fc2b7fe859 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -5,6 +5,7 @@ import { __ } from '~/locale'; import createFlash from '~/flash'; import { GlLoadingIcon } from '@gitlab/ui'; import PanelResizer from '~/vue_shared/components/panel_resizer.vue'; +import Mousetrap from 'mousetrap'; import eventHub from '../../notes/event_hub'; import CompareVersions from './compare_versions.vue'; import DiffFile from './diff_file.vue'; @@ -87,7 +88,7 @@ export default { emailPatchPath: state => state.diffs.emailPatchPath, }), ...mapState('diffs', ['showTreeList', 'isLoading', 'startVersion']), - ...mapGetters('diffs', ['isParallelView']), + ...mapGetters('diffs', ['isParallelView', 'currentDiffIndex']), ...mapGetters(['isNotesFetched', 'getNoteableData']), targetBranch() { return { @@ -149,6 +150,7 @@ export default { }, beforeDestroy() { eventHub.$off('fetchDiffData', this.fetchData); + this.removeEventListeners(); }, methods: { ...mapActions(['startTaskList']), @@ -159,6 +161,7 @@ export default { 'assignDiscussionsToDiff', 'setHighlightedRow', 'cacheTreeListWidth', + 'scrollToFile', ]), fetchData() { this.fetchDiffFiles() @@ -197,7 +200,35 @@ export default { this.$nextTick(() => { window.mrTabs.resetViewContainer(); window.mrTabs.expandViewContainer(this.showTreeList); + this.setEventListeners(); }); + } else { + this.removeEventListeners(); + } + }, + setEventListeners() { + Mousetrap.bind(['[', 'k', ']', 'j'], (e, combo) => { + switch (combo) { + case '[': + case 'k': + this.jumpToFile(-1); + break; + case ']': + case 'j': + this.jumpToFile(+1); + break; + default: + break; + } + }); + }, + removeEventListeners() { + Mousetrap.unbind(['[', 'k', ']', 'j']); + }, + jumpToFile(step) { + const targetIndex = this.currentDiffIndex + step; + if (targetIndex >= 0 && targetIndex < this.diffFiles.length) { + this.scrollToFile(this.diffFiles[targetIndex].file_path); } }, }, diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 82ff2e3be76..c40775c3259 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -52,7 +52,9 @@ export const fetchDiffFiles = ({ state, commit }) => { }; export const setHighlightedRow = ({ commit }, lineCode) => { + const fileHash = lineCode.split('_')[0]; commit(types.SET_HIGHLIGHTED_ROW, lineCode); + commit(types.UPDATE_CURRENT_DIFF_FILE_ID, fileHash); }; // This is adding line discussions to the actual lines in the diff tree @@ -262,8 +264,6 @@ export const scrollToFile = ({ state, commit }, path) => { document.location.hash = fileHash; commit(types.UPDATE_CURRENT_DIFF_FILE_ID, fileHash); - - setTimeout(() => commit(types.UPDATE_CURRENT_DIFF_FILE_ID, ''), 1000); }; export const toggleShowTreeList = ({ commit, state }) => { diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index 4e7e5306995..bc27e263bff 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -100,5 +100,12 @@ export const diffFilesLength = state => state.diffFiles.length; export const getCommentFormForDiffFile = state => fileHash => state.commentForms.find(form => form.fileHash === fileHash); +/** + * Returns index of a currently selected diff in diffFiles + * @returns {number} + */ +export const currentDiffIndex = state => + Math.max(0, state.diffFiles.findIndex(diff => diff.file_hash === state.currentDiffFileId)); + // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/views/help/_shortcuts.html.haml b/app/views/help/_shortcuts.html.haml index 28ffb2dd63c..efb3815b257 100644 --- a/app/views/help/_shortcuts.html.haml +++ b/app/views/help/_shortcuts.html.haml @@ -356,6 +356,18 @@ %td.shortcut %kbd l %td Change Label + %tr + %td.shortcut + %kbd ] + \/ + %kbd j + %td Move to next file + %tr + %td.shortcut + %kbd [ + \/ + %kbd k + %td Move to previous file %tbody.hidden-shortcut{ style: 'display:none' } %tr %th diff --git a/changelogs/unreleased/48798-keybinding-mr-diff.yml b/changelogs/unreleased/48798-keybinding-mr-diff.yml new file mode 100644 index 00000000000..3ef3f07f27c --- /dev/null +++ b/changelogs/unreleased/48798-keybinding-mr-diff.yml @@ -0,0 +1,5 @@ +--- +title: Next/previous navigation between files in MR review +merge_request: 25355 +author: +type: added
\ No newline at end of file diff --git a/doc/workflow/shortcuts.md b/doc/workflow/shortcuts.md index 7359e1c6119..6ed6b0bda66 100644 --- a/doc/workflow/shortcuts.md +++ b/doc/workflow/shortcuts.md @@ -82,6 +82,8 @@ You can see GitLab's keyboard shortcuts by using 'shift + ?' | <kbd>r</kbd> | Reply (quoting selected text) | | <kbd>e</kbd> | Edit issue/merge request | | <kbd>l</kbd> | Change label | +| <kbd>]</kbd> or <kbd>j</kbd> | Move to next file | +| <kbd>[</kbd> or <kbd>k</kbd> | Move to previous file | ## Wiki pages diff --git a/spec/javascripts/diffs/components/app_spec.js b/spec/javascripts/diffs/components/app_spec.js index 5abdfe695d0..bce6113f75a 100644 --- a/spec/javascripts/diffs/components/app_spec.js +++ b/spec/javascripts/diffs/components/app_spec.js @@ -4,12 +4,13 @@ import { TEST_HOST } from 'spec/test_constants'; import App from '~/diffs/components/app.vue'; import NoChanges from '~/diffs/components/no_changes.vue'; import DiffFile from '~/diffs/components/diff_file.vue'; +import Mousetrap from 'mousetrap'; import createDiffsStore from '../create_diffs_store'; describe('diffs/components/app', () => { const oldMrTabs = window.mrTabs; let store; - let vm; + let wrapper; function createComponent(props = {}, extendStore = () => {}) { const localVue = createLocalVue(); @@ -21,7 +22,7 @@ describe('diffs/components/app', () => { extendStore(store); - vm = shallowMount(localVue.extend(App), { + wrapper = shallowMount(localVue.extend(App), { localVue, propsData: { endpoint: `${TEST_HOST}/diff/endpoint`, @@ -38,7 +39,6 @@ describe('diffs/components/app', () => { // setup globals (needed for component to mount :/) window.mrTabs = jasmine.createSpyObj('mrTabs', ['resetViewContainer']); window.mrTabs.expandViewContainer = jasmine.createSpy(); - window.location.hash = 'ABC_123'; }); afterEach(() => { @@ -46,25 +46,44 @@ describe('diffs/components/app', () => { window.mrTabs = oldMrTabs; // reset component - vm.destroy(); + wrapper.destroy(); }); it('does not show commit info', () => { createComponent(); - expect(vm.contains('.blob-commit-info')).toBe(false); + expect(wrapper.contains('.blob-commit-info')).toBe(false); }); - it('sets highlighted row if hash exists in location object', done => { - createComponent({ - shouldShow: true, + describe('row highlighting', () => { + beforeEach(() => { + window.location.hash = 'ABC_123'; }); - // Component uses $nextTick so we wait until that has finished - setTimeout(() => { - expect(store.state.diffs.highlightedRow).toBe('ABC_123'); + it('sets highlighted row if hash exists in location object', done => { + createComponent({ + shouldShow: true, + }); - done(); + // Component uses $nextTick so we wait until that has finished + setTimeout(() => { + expect(store.state.diffs.highlightedRow).toBe('ABC_123'); + + done(); + }); + }); + + it('marks current diff file based on currently highlighted row', done => { + createComponent({ + shouldShow: true, + }); + + // Component uses $nextTick so we wait until that has finished + setTimeout(() => { + expect(store.state.diffs.currentDiffFileId).toBe('ABC'); + + done(); + }); }); }); @@ -76,7 +95,7 @@ describe('diffs/components/app', () => { it('sets initial width when no localStorage has been set', () => { createComponent(); - expect(vm.vm.treeWidth).toEqual(320); + expect(wrapper.vm.treeWidth).toEqual(320); }); it('sets initial width to localStorage size', () => { @@ -84,13 +103,26 @@ describe('diffs/components/app', () => { createComponent(); - expect(vm.vm.treeWidth).toEqual(200); + expect(wrapper.vm.treeWidth).toEqual(200); }); it('sets width of tree list', () => { createComponent(); - expect(vm.find('.js-diff-tree-list').element.style.width).toEqual('320px'); + expect(wrapper.find('.js-diff-tree-list').element.style.width).toEqual('320px'); + }); + }); + + it('marks current diff file based on currently highlighted row', done => { + createComponent({ + shouldShow: true, + }); + + // Component uses $nextTick so we wait until that has finished + setTimeout(() => { + expect(store.state.diffs.currentDiffFileId).toBe('ABC'); + + done(); }); }); @@ -98,7 +130,7 @@ describe('diffs/components/app', () => { it('renders empty state when no diff files exist', () => { createComponent(); - expect(vm.contains(NoChanges)).toBe(true); + expect(wrapper.contains(NoChanges)).toBe(true); }); it('does not render empty state when diff files exist', () => { @@ -108,8 +140,8 @@ describe('diffs/components/app', () => { }); }); - expect(vm.contains(NoChanges)).toBe(false); - expect(vm.findAll(DiffFile).length).toBe(1); + expect(wrapper.contains(NoChanges)).toBe(false); + expect(wrapper.findAll(DiffFile).length).toBe(1); }); it('does not render empty state when versions match', () => { @@ -118,7 +150,161 @@ describe('diffs/components/app', () => { store.state.diffs.mergeRequestDiff = { version_index: 1 }; }); - expect(vm.contains(NoChanges)).toBe(false); + expect(wrapper.contains(NoChanges)).toBe(false); + }); + }); + + describe('keyboard shortcut navigation', () => { + const mappings = { + '[': -1, + k: -1, + ']': +1, + j: +1, + }; + let spy; + + describe('visible app', () => { + beforeEach(() => { + spy = jasmine.createSpy('spy'); + + createComponent({ + shouldShow: true, + }); + wrapper.setMethods({ + jumpToFile: spy, + }); + }); + + it('calls `jumpToFile()` with correct parameter whenever pre-defined key is pressed', done => { + wrapper.vm + .$nextTick() + .then(() => { + Object.keys(mappings).forEach(function(key) { + Mousetrap.trigger(key); + + expect(spy.calls.mostRecent().args).toEqual([mappings[key]]); + }); + + expect(spy.calls.count()).toEqual(Object.keys(mappings).length); + }) + .then(done) + .catch(done.fail); + }); + + it('does not call `jumpToFile()` when unknown key is pressed', done => { + wrapper.vm + .$nextTick() + .then(() => { + Mousetrap.trigger('d'); + + expect(spy).not.toHaveBeenCalled(); + }) + .then(done) + .catch(done.fail); + }); + }); + + describe('hideen app', () => { + beforeEach(() => { + spy = jasmine.createSpy('spy'); + + createComponent({ + shouldShow: false, + }); + wrapper.setMethods({ + jumpToFile: spy, + }); + }); + + it('stops calling `jumpToFile()` when application is hidden', done => { + wrapper.vm + .$nextTick() + .then(() => { + Object.keys(mappings).forEach(function(key) { + Mousetrap.trigger(key); + + expect(spy).not.toHaveBeenCalled(); + }); + }) + .then(done) + .catch(done.fail); + }); + }); + }); + + describe('jumpToFile', () => { + let spy; + + beforeEach(() => { + spy = jasmine.createSpy(); + + createComponent({}, () => { + store.state.diffs.diffFiles = [ + { file_hash: '111', file_path: '111.js' }, + { file_hash: '222', file_path: '222.js' }, + { file_hash: '333', file_path: '333.js' }, + ]; + }); + + wrapper.setMethods({ + scrollToFile: spy, + }); + }); + + afterEach(() => { + wrapper.destroy(); + }); + + it('jumps to next and previous files in the list', done => { + wrapper.vm + .$nextTick() + .then(() => { + wrapper.vm.jumpToFile(+1); + + expect(spy.calls.mostRecent().args).toEqual(['222.js']); + store.state.diffs.currentDiffFileId = '222'; + wrapper.vm.jumpToFile(+1); + + expect(spy.calls.mostRecent().args).toEqual(['333.js']); + store.state.diffs.currentDiffFileId = '333'; + wrapper.vm.jumpToFile(-1); + + expect(spy.calls.mostRecent().args).toEqual(['222.js']); + }) + .then(done) + .catch(done.fail); + }); + + it('does not jump to previous file from the first one', done => { + wrapper.vm + .$nextTick() + .then(() => { + store.state.diffs.currentDiffFileId = '333'; + + expect(wrapper.vm.currentDiffIndex).toEqual(2); + + wrapper.vm.jumpToFile(+1); + + expect(wrapper.vm.currentDiffIndex).toEqual(2); + expect(spy).not.toHaveBeenCalled(); + }) + .then(done) + .catch(done.fail); + }); + + it('does not jump to next file from the last one', done => { + wrapper.vm + .$nextTick() + .then(() => { + expect(wrapper.vm.currentDiffIndex).toEqual(0); + + wrapper.vm.jumpToFile(-1); + + expect(wrapper.vm.currentDiffIndex).toEqual(0); + expect(spy).not.toHaveBeenCalled(); + }) + .then(done) + .catch(done.fail); }); }); }); diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index acff80bca62..e47c7906fcb 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -100,9 +100,10 @@ describe('DiffsStoreActions', () => { }); describe('setHighlightedRow', () => { - it('should set lineHash and fileHash of highlightedRow', () => { + it('should mark currently selected diff and set lineHash and fileHash of highlightedRow', () => { testAction(setHighlightedRow, 'ABC_123', {}, [ { type: types.SET_HIGHLIGHTED_ROW, payload: 'ABC_123' }, + { type: types.UPDATE_CURRENT_DIFF_FILE_ID, payload: 'ABC' }, ]); }); }); @@ -713,22 +714,6 @@ describe('DiffsStoreActions', () => { expect(commit).toHaveBeenCalledWith(types.UPDATE_CURRENT_DIFF_FILE_ID, 'test'); }); - - it('resets currentDiffId after timeout', () => { - const state = { - treeEntries: { - path: { - fileHash: 'test', - }, - }, - }; - - scrollToFile({ state, commit }, 'path'); - - jasmine.clock().tick(1000); - - expect(commit.calls.argsFor(1)).toEqual([types.UPDATE_CURRENT_DIFF_FILE_ID, '']); - }); }); describe('toggleShowTreeList', () => { diff --git a/spec/javascripts/diffs/store/getters_spec.js b/spec/javascripts/diffs/store/getters_spec.js index 0ab88e6b2aa..eab5703dfb2 100644 --- a/spec/javascripts/diffs/store/getters_spec.js +++ b/spec/javascripts/diffs/store/getters_spec.js @@ -270,4 +270,24 @@ describe('Diffs Module Getters', () => { expect(getters.diffFilesLength(localState)).toBe(2); }); }); + + describe('currentDiffIndex', () => { + it('returns index of currently selected diff in diffList', () => { + localState.diffFiles = [{ file_hash: '111' }, { file_hash: '222' }, { file_hash: '333' }]; + localState.currentDiffFileId = '222'; + + expect(getters.currentDiffIndex(localState)).toEqual(1); + + localState.currentDiffFileId = '333'; + + expect(getters.currentDiffIndex(localState)).toEqual(2); + }); + + it('returns 0 if no diff is selected yet or diff is not found', () => { + localState.diffFiles = [{ file_hash: '111' }, { file_hash: '222' }, { file_hash: '333' }]; + localState.currentDiffFileId = ''; + + expect(getters.currentDiffIndex(localState)).toEqual(0); + }); + }); }); |