Skip to content
Snippets Groups Projects
Commit 40ca181c authored by Kushal Pandya's avatar Kushal Pandya
Browse files

Merge branch 'ph/371557/contextAwareDiscussionsDiffs' into 'master'

Context aware discussion navigation on diffs tab

See merge request gitlab-org/gitlab!100602



Merged-by: default avatarKushal Pandya <kushalspandya@gmail.com>
Approved-by: default avatarKushal Pandya <kushalspandya@gmail.com>
Co-authored-by: default avatarPhil Hughes <me@iamphill.com>
parents 4c5ea813 fcf8f026
No related branches found
No related tags found
1 merge request!100602Context aware discussion navigation on diffs tab
Pipeline #670697936 failed
......@@ -167,7 +167,7 @@ export const contentTop = () => {
return size;
},
() => getOuterHeight('.merge-request-tabs'),
() => getOuterHeight('.merge-request-sticky-header, .merge-request-tabs'),
() => getOuterHeight('.js-diff-files-changed'),
() => getOuterHeight('.issue-sticky-header.gl-fixed'),
({ desktop }) => {
......@@ -175,7 +175,9 @@ export const contentTop = () => {
let size;
if (desktop && diffsTabIsActive) {
size = getOuterHeight('.diff-file .file-title-flex-parent:not([style="display:none"])');
size = getOuterHeight(
'.diffs .diff-file .file-title-flex-parent:not([style="display:none"])',
);
}
return size;
......
import { mapGetters, mapActions, mapState } from 'vuex';
import { scrollToElementWithContext, scrollToElement, contentTop } from '~/lib/utils/common_utils';
import { updateHistory } from '~/lib/utils/url_utility';
import eventHub from '../event_hub';
/**
* @param {string} selector
* @returns {boolean}
*/
function scrollTo(selector, { withoutContext = false, offset = 0 } = {}) {
const el = document.querySelector(selector);
const scrollFunction = withoutContext ? scrollToElement : scrollToElementWithContext;
if (el) {
scrollFunction(el, {
behavior: 'auto',
offset,
});
return true;
}
return false;
}
function updateUrlWithNoteId(noteId) {
const newHistoryEntry = {
state: null,
title: window.title,
url: `#note_${noteId}`,
replace: true,
};
if (noteId) {
// Temporarily mask the ID to avoid the browser default
// scrolling taking over which is broken with virtual
// scrolling enabled.
const note = document.querySelector(`#note_${noteId}`);
note?.setAttribute('id', `masked::${note.id}`);
// Update the hash now that the ID "doesn't exist" in the page
updateHistory(newHistoryEntry);
// Unmask the note's ID
note?.setAttribute('id', `note_${noteId}`);
}
}
/**
* @param {object} self Component instance with mixin applied
* @param {string} id Discussion id we are jumping to
*/
function diffsJump({ expandDiscussion }, id, firstNoteId) {
const selector = `ul.notes[data-discussion-id="${id}"]`;
eventHub.$once('scrollToDiscussion', () => {
scrollTo(selector);
// Wait for the discussion scroll before updating to the more specific ID
setTimeout(() => updateUrlWithNoteId(firstNoteId), 0);
});
expandDiscussion({ discussionId: id });
}
/**
* @param {object} self Component instance with mixin applied
* @param {string} id Discussion id we are jumping to
* @returns {boolean}
*/
function discussionJump({ expandDiscussion }, id) {
const selector = `div.discussion[data-discussion-id="${id}"]`;
expandDiscussion({ discussionId: id });
return scrollTo(selector, {
withoutContext: true,
offset: window.gon?.features?.movedMrSidebar ? -28 : 0,
});
}
/**
* @param {object} self Component instance with mixin applied
* @param {string} id Discussion id we are jumping to
*/
function switchToDiscussionsTabAndJumpTo(self, id) {
window.mrTabs.eventHub.$once('MergeRequestTabChange', () => {
setTimeout(() => discussionJump(self, id), 0);
});
window.mrTabs.tabShown('show');
}
/**
* @param {object} self Component instance with mixin applied
* @param {object} discussion Discussion we are jumping to
*/
function jumpToDiscussion(self, discussion) {
const { id, diff_discussion: isDiffDiscussion, notes } = discussion;
const firstNoteId = notes?.[0]?.id;
if (id) {
const activeTab = window.mrTabs.currentAction;
if (activeTab === 'diffs' && isDiffDiscussion) {
diffsJump(self, id, firstNoteId);
} else {
switchToDiscussionsTabAndJumpTo(self, id);
}
}
}
/**
* @param {object} self Component instance with mixin applied
* @param {function} fn Which function used to get the target discussion's id
*/
function handleDiscussionJump(self, fn) {
const isDiffView = window.mrTabs.currentAction === 'diffs';
const targetId = fn(self.currentDiscussionId, isDiffView);
const discussion = self.getDiscussion(targetId);
const discussionFilePath = discussion?.diff_file?.file_path;
window.location.hash = '';
if (discussionFilePath) {
self.scrollToFile({
path: discussionFilePath,
});
}
self.$nextTick(() => {
jumpToDiscussion(self, discussion);
self.setCurrentDiscussionId(targetId);
});
}
import { scrollToElement, contentTop } from '~/lib/utils/common_utils';
function getAllDiscussionElements() {
const containerEl = window.mrTabs?.currentAction === 'diffs' ? '.diffs' : '.notes';
return Array.from(
document.querySelectorAll('[data-discussion-id]:not([data-discussion-resolved])'),
document.querySelectorAll(
`${containerEl} div[data-discussion-id]:not([data-discussion-resolved])`,
),
);
}
......@@ -182,14 +58,10 @@ function getPreviousDiscussion() {
}
function handleJumpForBothPages(getDiscussion, ctx, fn, scrollOptions) {
if (window.mrTabs.currentAction !== 'show') {
handleDiscussionJump(ctx, fn);
} else {
const discussion = getDiscussion();
const id = discussion.dataset.discussionId;
ctx.expandDiscussion({ discussionId: id });
scrollToElement(discussion, scrollOptions);
}
const discussion = getDiscussion();
const id = discussion.dataset.discussionId;
ctx.expandDiscussion({ discussionId: id });
scrollToElement(discussion, scrollOptions);
}
export default {
......@@ -205,9 +77,11 @@ export default {
},
methods: {
...mapActions(['expandDiscussion', 'setCurrentDiscussionId']),
...mapActions('diffs', ['scrollToFile']),
...mapActions('diffs', ['scrollToFile', 'disableVirtualScroller']),
async jumpToNextDiscussion(scrollOptions) {
await this.disableVirtualScroller();
jumpToNextDiscussion(scrollOptions) {
handleJumpForBothPages(
getNextDiscussion,
this,
......@@ -216,7 +90,9 @@ export default {
);
},
jumpToPreviousDiscussion(scrollOptions) {
async jumpToPreviousDiscussion(scrollOptions) {
await this.disableVirtualScroller();
handleJumpForBothPages(
getPreviousDiscussion,
this,
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe 'User jumps to the next unresolved discussion', :js do
let(:project) { create(:project, :repository) }
let(:merge_request) do
create(:merge_request_with_diffs, source_project: project, target_project: project, source_branch: 'merge-test')
end
let(:user) { create(:user) }
before do
create(:discussion_note, noteable: merge_request, project: project, author: user)
project.add_maintainer(user)
sign_in(user)
visit(diffs_project_merge_request_path(project, merge_request))
wait_for_requests
end
it 'jumps to overview tab' do
find('.discussion-next-btn').click
expect(page).to have_css('.notes-tab.active')
end
end
......@@ -4,7 +4,6 @@ import Vuex from 'vuex';
import { setHTMLFixture } from 'helpers/fixtures';
import createEventHub from '~/helpers/event_hub_factory';
import * as utils from '~/lib/utils/common_utils';
import eventHub from '~/notes/event_hub';
import discussionNavigation from '~/notes/mixins/discussion_navigation';
import notesModule from '~/notes/stores/modules';
......@@ -35,13 +34,15 @@ describe('Discussion navigation mixin', () => {
beforeEach(() => {
setHTMLFixture(
[...'abcde']
`<div class="notes">
${[...'abcde']
.map(
(id) =>
`<ul class="notes" data-discussion-id="${id}"></ul>
<div class="discussion" data-discussion-id="${id}"></div>`,
)
.join(''),
.join('')}
</div>`,
);
jest.spyOn(utils, 'scrollToElementWithContext');
......@@ -58,7 +59,7 @@ describe('Discussion navigation mixin', () => {
},
diffs: {
namespaced: true,
actions: { scrollToFile },
actions: { scrollToFile, disableVirtualScroller: () => {} },
state: { diffFiles: [] },
},
},
......@@ -73,9 +74,6 @@ describe('Discussion navigation mixin', () => {
jest.clearAllMocks();
});
const findDiscussion = (selector, id) =>
document.querySelector(`${selector}[data-discussion-id="${id}"]`);
describe('jumpToFirstUnresolvedDiscussion method', () => {
let vm;
......@@ -110,14 +108,14 @@ describe('Discussion navigation mixin', () => {
});
describe.each`
fn | args | currentId | expected
${'jumpToNextDiscussion'} | ${[]} | ${null} | ${'a'}
${'jumpToNextDiscussion'} | ${[]} | ${'a'} | ${'c'}
${'jumpToNextDiscussion'} | ${[]} | ${'e'} | ${'a'}
${'jumpToPreviousDiscussion'} | ${[]} | ${null} | ${'e'}
${'jumpToPreviousDiscussion'} | ${[]} | ${'e'} | ${'c'}
${'jumpToPreviousDiscussion'} | ${[]} | ${'c'} | ${'a'}
`('$fn (args = $args, currentId = $currentId)', ({ fn, args, currentId, expected }) => {
fn | args | currentId
${'jumpToNextDiscussion'} | ${[]} | ${null}
${'jumpToNextDiscussion'} | ${[]} | ${'a'}
${'jumpToNextDiscussion'} | ${[]} | ${'e'}
${'jumpToPreviousDiscussion'} | ${[]} | ${null}
${'jumpToPreviousDiscussion'} | ${[]} | ${'e'}
${'jumpToPreviousDiscussion'} | ${[]} | ${'c'}
`('$fn (args = $args, currentId = $currentId)', ({ fn, args, currentId }) => {
beforeEach(() => {
store.state.notes.currentDiscussionId = currentId;
});
......@@ -130,125 +128,18 @@ describe('Discussion navigation mixin', () => {
await nextTick();
});
it('expands discussion', () => {
expect(expandDiscussion).toHaveBeenCalled();
});
it('scrolls to element', () => {
expect(utils.scrollToElement).toHaveBeenCalled();
});
});
describe('on `diffs` active tab', () => {
beforeEach(async () => {
window.mrTabs.currentAction = 'diffs';
wrapper.vm[fn](...args);
it('expands discussion', async () => {
await nextTick();
});
it('sets current discussion', () => {
expect(store.state.notes.currentDiscussionId).toEqual(expected);
});
it('expands discussion', () => {
expect(expandDiscussion).toHaveBeenCalled();
});
it('scrolls when scrollToDiscussion is emitted', () => {
expect(utils.scrollToElementWithContext).not.toHaveBeenCalled();
eventHub.$emit('scrollToDiscussion');
expect(utils.scrollToElementWithContext).toHaveBeenCalledWith(
findDiscussion('ul.notes', expected),
{ behavior: 'auto', offset: 0 },
);
});
});
describe('on `other` active tab', () => {
beforeEach(async () => {
window.mrTabs.currentAction = 'other';
wrapper.vm[fn](...args);
it('scrolls to element', async () => {
await nextTick();
});
it('sets current discussion', () => {
expect(store.state.notes.currentDiscussionId).toEqual(expected);
});
it('does not expand discussion yet', () => {
expect(expandDiscussion).not.toHaveBeenCalled();
});
it('shows mrTabs', () => {
expect(window.mrTabs.tabShown).toHaveBeenCalledWith('show');
});
describe('when tab is changed', () => {
beforeEach(() => {
window.mrTabs.eventHub.$emit('MergeRequestTabChange');
jest.runAllTimers();
});
it('expands discussion', () => {
expect(expandDiscussion).toHaveBeenCalledWith(expect.anything(), {
discussionId: expected,
});
});
it('scrolls to discussion', () => {
expect(utils.scrollToElement).toHaveBeenCalledWith(
findDiscussion('div.discussion', expected),
{ behavior: 'auto', offset: 0 },
);
});
expect(utils.scrollToElement).toHaveBeenCalled();
});
});
});
describe('virtual scrolling feature', () => {
beforeEach(() => {
jest.spyOn(store, 'dispatch');
store.state.notes.currentDiscussionId = 'a';
window.location.hash = 'test';
});
afterEach(() => {
window.gon = {};
window.location.hash = '';
});
it('resets location hash', async () => {
wrapper.vm.jumpToNextDiscussion();
await nextTick();
expect(window.location.hash).toBe('');
});
it.each`
tabValue
${'diffs'}
${'other'}
`(
'calls scrollToFile with setHash as $hashValue when the tab is $tabValue',
async ({ tabValue }) => {
window.mrTabs.currentAction = tabValue;
wrapper.vm.jumpToNextDiscussion();
await nextTick();
expect(store.dispatch).toHaveBeenCalledWith('diffs/scrollToFile', {
path: 'test.js',
});
},
);
});
});
});
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment