Commit 4fdd8762 authored by Paul Slaughter's avatar Paul Slaughter

Merge branch '59590-keyboard-shortcut-for-jump-to-next-unresolved-discussion' into 'master'

Resolve "Keyboard shortcut for "jump to NEXT unresolved discussion""

See merge request gitlab-org/gitlab-ce!30144
parents ff81c0a3 eba44228
......@@ -109,7 +109,7 @@ export const toggleLineDiscussions = ({ commit }, options) => {
export const renderFileForDiscussionId = ({ commit, rootState, state }, discussionId) => {
const discussion = rootState.notes.discussions.find(d => d.id === discussionId);
if (discussion) {
if (discussion && discussion.diff_file) {
const file = state.diffFiles.find(f => f.file_hash === discussion.diff_file.file_hash);
if (file) {
......
......@@ -3,6 +3,7 @@ import Vue from 'vue';
import { mapActions, mapState, mapGetters } from 'vuex';
import store from 'ee_else_ce/mr_notes/stores';
import notesApp from '../notes/components/notes_app.vue';
import discussionKeyboardNavigator from '../notes/components/discussion_keyboard_navigator.vue';
export default () => {
// eslint-disable-next-line no-new
......@@ -56,7 +57,10 @@ export default () => {
},
},
render(createElement) {
return createElement('notes-app', {
const isDiffView = this.activeTab === 'diffs';
return createElement(discussionKeyboardNavigator, { props: { isDiffView } }, [
createElement('notes-app', {
props: {
noteableData: this.noteableData,
notesData: this.notesData,
......@@ -64,7 +68,8 @@ export default () => {
shouldShow: this.activeTab === 'show',
helpPagePath: this.helpPagePath,
},
});
}),
]);
},
});
};
<script>
/* global Mousetrap */
import 'mousetrap';
import { mapGetters, mapActions } from 'vuex';
import discussionNavigation from '~/notes/mixins/discussion_navigation';
export default {
mixins: [discussionNavigation],
props: {
isDiffView: {
type: Boolean,
required: false,
default: false,
},
},
data() {
return {
currentDiscussionId: null,
};
},
computed: {
...mapGetters(['nextUnresolvedDiscussionId', 'previousUnresolvedDiscussionId']),
},
mounted() {
Mousetrap.bind('n', () => this.jumpToNextDiscussion());
Mousetrap.bind('p', () => this.jumpToPreviousDiscussion());
},
methods: {
...mapActions(['expandDiscussion']),
jumpToNextDiscussion() {
const nextId = this.nextUnresolvedDiscussionId(this.currentDiscussionId, this.isDiffView);
this.jumpToDiscussion(nextId);
this.currentDiscussionId = nextId;
},
jumpToPreviousDiscussion() {
const prevId = this.previousUnresolvedDiscussionId(this.currentDiscussionId, this.isDiffView);
this.jumpToDiscussion(prevId);
this.currentDiscussionId = prevId;
},
},
render() {
return this.$slots.default;
},
};
</script>
......@@ -183,6 +183,15 @@ export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, dif
return slicedIds.length ? idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0] : idsOrdered[0];
};
export const previousUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => {
const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder);
const currentIndex = idsOrdered.indexOf(discussionId);
const slicedIds = idsOrdered.slice(currentIndex - 1, currentIndex);
// Get the last ID if there is none after the currentIndex
return slicedIds.length ? slicedIds[0] : idsOrdered[idsOrdered.length - 1];
};
// @param {Boolean} diffOrder - is ordered by diff?
export const firstUnresolvedDiscussionId = (state, getters) => diffOrder => {
if (diffOrder) {
......
......@@ -332,7 +332,7 @@
%td.shortcut
%kbd l
%td Change Label
%tbody.hidden-shortcut{ style: 'display:none' }
%tbody.hidden-shortcut.merge_requests{ style: 'display:none' }
%tr
%th
%th Merge Requests
......@@ -368,6 +368,14 @@
\/
%kbd k
%td Move to previous file
%tr
%td.shortcut
%kbd n
%td= _("Move to next unresolved discussion")
%tr
%td.shortcut
%kbd p
%td= _("Move to previous unresolved discussion")
%tbody.hidden-shortcut{ style: 'display:none' }
%tr
%th
......
---
title: Resolve Keyboard shortcut for jump to NEXT unresolved discussion
merge_request: 30144
author:
type: added
......@@ -88,6 +88,11 @@ Jump button next to the Reply field on a thread.
You can also jump to the first unresolved thread from the button next to the
resolved threads tracker.
You can also use keyboard shortcuts to navigate among threads:
- Use <kbd>n</kbd> to jump to the next unresolved thread.
- Use <kbd>p</kbd> to jump to the previous unresolved thread.
!["8/9 threads resolved"](img/threads_resolved.png)
### Marking a comment or thread as resolved
......
......@@ -84,6 +84,8 @@ You can see GitLab's keyboard shortcuts by using <kbd>shift</kbd> + <kbd>?</kbd>
| <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 |
| <kbd>n</kbd> | Move to next unresolved discussion |
| <kbd>p</kbd> | Move to previous unresolved discussion |
## Epics **(ULTIMATE)**
......
......@@ -6952,6 +6952,12 @@ msgstr ""
msgid "Move this issue to another project."
msgstr ""
msgid "Move to next unresolved discussion"
msgstr ""
msgid "Move to previous unresolved discussion"
msgstr ""
msgid "MoveIssue|Cannot move issue due to insufficient permissions!"
msgstr ""
......
/* global Mousetrap */
import 'mousetrap';
import { shallowMount, createLocalVue } from '@vue/test-utils';
import Vuex from 'vuex';
import DiscussionKeyboardNavigator from '~/notes/components/discussion_keyboard_navigator.vue';
import notesModule from '~/notes/stores/modules';
const localVue = createLocalVue();
localVue.use(Vuex);
const NEXT_ID = 'abc123';
const PREV_ID = 'def456';
const NEXT_DIFF_ID = 'abc123_diff';
const PREV_DIFF_ID = 'def456_diff';
describe('notes/components/discussion_keyboard_navigator', () => {
let storeOptions;
let wrapper;
let store;
const createComponent = (options = {}) => {
store = new Vuex.Store(storeOptions);
wrapper = shallowMount(DiscussionKeyboardNavigator, {
localVue,
store,
...options,
});
wrapper.vm.jumpToDiscussion = jest.fn();
};
beforeEach(() => {
const notes = notesModule();
notes.getters.nextUnresolvedDiscussionId = () => (currId, isDiff) =>
isDiff ? NEXT_DIFF_ID : NEXT_ID;
notes.getters.previousUnresolvedDiscussionId = () => (currId, isDiff) =>
isDiff ? PREV_DIFF_ID : PREV_ID;
storeOptions = {
modules: {
notes,
},
};
});
afterEach(() => {
wrapper.destroy();
storeOptions = null;
store = null;
});
describe.each`
isDiffView | expectedNextId | expectedPrevId
${true} | ${NEXT_DIFF_ID} | ${PREV_DIFF_ID}
${false} | ${NEXT_ID} | ${PREV_ID}
`('when isDiffView is $isDiffView', ({ isDiffView, expectedNextId, expectedPrevId }) => {
beforeEach(() => {
createComponent({ propsData: { isDiffView } });
});
it('calls jumpToNextDiscussion when pressing `n`', () => {
Mousetrap.trigger('n');
expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith(expectedNextId);
expect(wrapper.vm.currentDiscussionId).toEqual(expectedNextId);
});
it('calls jumpToPreviousDiscussion when pressing `p`', () => {
Mousetrap.trigger('p');
expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith(expectedPrevId);
expect(wrapper.vm.currentDiscussionId).toEqual(expectedPrevId);
});
});
});
......@@ -256,6 +256,54 @@ describe('Getters Notes Store', () => {
});
});
describe('previousUnresolvedDiscussionId', () => {
describe('with unresolved discussions', () => {
const localGetters = {
unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'],
};
it('with bogus returns falsey', () => {
expect(getters.previousUnresolvedDiscussionId(state, localGetters)('bogus')).toBe('456');
});
[
{ id: '123', expected: '789' },
{ id: '456', expected: '123' },
{ id: '789', expected: '456' },
].forEach(({ id, expected }) => {
it(`with ${id}, returns previous value`, () => {
expect(getters.previousUnresolvedDiscussionId(state, localGetters)(id)).toBe(expected);
});
});
});
describe('with 1 unresolved discussion', () => {
const localGetters = {
unresolvedDiscussionsIdsOrdered: () => ['123'],
};
it('with bogus returns id', () => {
expect(getters.previousUnresolvedDiscussionId(state, localGetters)('bogus')).toBe('123');
});
it('with match, returns value', () => {
expect(getters.previousUnresolvedDiscussionId(state, localGetters)('123')).toEqual('123');
});
});
describe('with 0 unresolved discussions', () => {
const localGetters = {
unresolvedDiscussionsIdsOrdered: () => [],
};
it('returns undefined', () => {
expect(
getters.previousUnresolvedDiscussionId(state, localGetters)('bogus'),
).toBeUndefined();
});
});
});
describe('firstUnresolvedDiscussionId', () => {
const localGetters = {
unresolvedDiscussionsIdsByDate: ['123', '456'],
......
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