diff options
author | André Luís <me@andr3.net> | 2018-12-08 10:19:03 +0300 |
---|---|---|
committer | Phil Hughes <me@iamphill.com> | 2018-12-08 10:19:03 +0300 |
commit | 85daddbec99cc0578277946e0fb8aee1abfaa8c8 (patch) | |
tree | c09f9eb1eabeaaaaa2c62bafe6b167c37acb2dfe /app/assets | |
parent | f3299596c7d9d7b042859253606bab9972ed4a13 (diff) |
Resolve "Navigating unresolved discussions on Merge Request page"
Diffstat (limited to 'app/assets')
9 files changed, 105 insertions, 19 deletions
diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index 3b2a0d156ca..bed29efb253 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -4,6 +4,7 @@ import _ from 'underscore'; import { __, sprintf } from '~/locale'; import createFlash from '~/flash'; import { GlLoadingIcon } from '@gitlab/ui'; +import eventHub from '../../notes/event_hub'; import DiffFileHeader from './diff_file_header.vue'; import DiffContent from './diff_content.vue'; @@ -75,6 +76,9 @@ export default { } }, }, + created() { + eventHub.$on(`loadCollapsedDiff/${this.file.file_hash}`, this.handleLoadCollapsedDiff); + }, methods: { ...mapActions('diffs', ['loadCollapsedDiff', 'assignDiscussionsToDiff']), handleToggle() { diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 952963e0711..00a4bb6d3a3 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -3,8 +3,9 @@ import axios from '~/lib/utils/axios_utils'; import Cookies from 'js-cookie'; import createFlash from '~/flash'; import { s__ } from '~/locale'; -import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils'; +import { handleLocationHash, historyPushState, scrollToElement } from '~/lib/utils/common_utils'; import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility'; +import eventHub from '../../notes/event_hub'; import { getDiffPositionByLineCode, getNoteFormData } from './utils'; import * as types from './mutation_types'; import { @@ -53,6 +54,10 @@ export const assignDiscussionsToDiff = ( diffPositionByLineCode, }); }); + + Vue.nextTick(() => { + eventHub.$emit('scrollToDiscussion'); + }); }; export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => { @@ -60,6 +65,27 @@ export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => { commit(types.REMOVE_LINE_DISCUSSIONS_FOR_FILE, { fileHash: file_hash, lineCode: line_code, id }); }; +export const renderFileForDiscussionId = ({ commit, rootState, state }, discussionId) => { + const discussion = rootState.notes.discussions.find(d => d.id === discussionId); + + if (discussion) { + const file = state.diffFiles.find(f => f.file_hash === discussion.diff_file.file_hash); + + if (file) { + if (!file.renderIt) { + commit(types.RENDER_FILE, file); + } + + if (file.collapsed) { + eventHub.$emit(`loadCollapsedDiff/${file.file_hash}`); + scrollToElement(document.getElementById(file.file_hash)); + } else { + eventHub.$emit('scrollToDiscussion'); + } + } + } +}; + export const startRenderDiffsQueue = ({ state, commit }) => { const checkItem = () => new Promise(resolve => { diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 331fb052371..61314db1dbd 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -170,7 +170,7 @@ export default { } if (!file.parallel_diff_lines || !file.highlighted_diff_lines) { - file.discussions = file.discussions.concat(discussion); + file.discussions = (file.discussions || []).concat(discussion); } return file; diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index 040d0bc659e..9e22cdc04e9 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -192,8 +192,12 @@ export const contentTop = () => { const mrTabsHeight = $('.merge-request-tabs').height() || 0; const headerHeight = $('.navbar-gitlab').height() || 0; const diffFilesChanged = $('.js-diff-files-changed').height() || 0; + const diffFileLargeEnoughScreen = + 'matchMedia' in window ? window.matchMedia('min-width: 768') : true; + const diffFileTitleBar = + (diffFileLargeEnoughScreen && $('.diff-file .file-title-flex-parent:visible').height()) || 0; - return perfBar + mrTabsHeight + headerHeight + diffFilesChanged; + return perfBar + mrTabsHeight + headerHeight + diffFilesChanged + diffFileTitleBar; }; export const scrollToElement = element => { diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index f4991a41325..d4450c9f2c8 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -81,6 +81,7 @@ export default { 'nextUnresolvedDiscussionId', 'unresolvedDiscussionsCount', 'hasUnresolvedDiscussions', + 'showJumpToNextDiscussion', ]), author() { return this.initialDiscussion.author; @@ -121,6 +122,12 @@ export default { resolvedText() { return this.discussion.resolved_by_push ? __('Automatically resolved') : __('Resolved'); }, + shouldShowJumpToNextDiscussion() { + return this.showJumpToNextDiscussion( + this.discussion.id, + this.discussionsByDiffOrder ? 'diff' : 'discussion', + ); + }, shouldRenderDiffs() { return this.discussion.diff_discussion && this.renderDiffFile; }, @@ -418,7 +425,7 @@ Please check your network connection and try again.`; <icon name="issue-new" /> </a> </div> - <div v-if="hasUnresolvedDiscussions" class="btn-group" role="group"> + <div v-if="shouldShowJumpToNextDiscussion" class="btn-group" role="group"> <button v-gl-tooltip class="btn btn-default discussion-next-btn" diff --git a/app/assets/javascripts/notes/mixins/discussion_navigation.js b/app/assets/javascripts/notes/mixins/discussion_navigation.js index f7c4deee1f8..3d89d907777 100644 --- a/app/assets/javascripts/notes/mixins/discussion_navigation.js +++ b/app/assets/javascripts/notes/mixins/discussion_navigation.js @@ -1,29 +1,56 @@ import { scrollToElement } from '~/lib/utils/common_utils'; +import eventHub from '../../notes/event_hub'; export default { methods: { - jumpToDiscussion(id) { - if (id) { - const activeTab = window.mrTabs.currentAction; - const selector = - activeTab === 'diffs' - ? `ul.notes[data-discussion-id="${id}"]` - : `div.discussion[data-discussion-id="${id}"]`; - const el = document.querySelector(selector); + diffsJump(id) { + const selector = `ul.notes[data-discussion-id="${id}"]`; - if (activeTab === 'commits' || activeTab === 'pipelines') { - window.mrTabs.activateTab('show'); - } + eventHub.$once('scrollToDiscussion', () => { + const el = document.querySelector(selector); if (el) { - this.expandDiscussion({ discussionId: id }); - scrollToElement(el); + return true; } + + return false; + }); + + this.expandDiscussion({ discussionId: id }); + }, + discussionJump(id) { + const selector = `div.discussion[data-discussion-id="${id}"]`; + + const el = document.querySelector(selector); + + this.expandDiscussion({ discussionId: id }); + + if (el) { + scrollToElement(el); + + return true; } return false; }, + jumpToDiscussion(id) { + if (id) { + const activeTab = window.mrTabs.currentAction; + + if (activeTab === 'diffs') { + this.diffsJump(id); + } else if (activeTab === 'commits' || activeTab === 'pipelines') { + window.mrTabs.eventHub.$once('MergeRequestTabChange', () => { + setTimeout(() => this.discussionJump(id), 0); + }); + + window.mrTabs.tabShown('show'); + } else { + this.discussionJump(id); + } + } + }, }, }; diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index b4befdd6e4a..4716ab52333 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -17,7 +17,13 @@ import { __ } from '~/locale'; let eTagPoll; -export const expandDiscussion = ({ commit }, data) => commit(types.EXPAND_DISCUSSION, data); +export const expandDiscussion = ({ commit, dispatch }, data) => { + if (data.discussionId) { + dispatch('diffs/renderFileForDiscussionId', data.discussionId, { root: true }); + } + + commit(types.EXPAND_DISCUSSION, data); +}; export const collapseDiscussion = ({ commit }, data) => commit(types.COLLAPSE_DISCUSSION, data); diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 2ed8aac059a..0ffc0cb2593 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -57,6 +57,17 @@ export const unresolvedDiscussionsCount = state => state.unresolvedDiscussionsCo export const resolvableDiscussionsCount = state => state.resolvableDiscussionsCount; export const hasUnresolvedDiscussions = state => state.hasUnresolvedDiscussions; +export const showJumpToNextDiscussion = (state, getters) => (discussionId, mode = 'discussion') => { + const orderedDiffs = + mode !== 'discussion' + ? getters.unresolvedDiscussionsIdsByDiff + : getters.unresolvedDiscussionsIdsByDate; + + const indexOf = orderedDiffs.indexOf(discussionId); + + return indexOf !== -1 && indexOf < orderedDiffs.length - 1; +}; + export const isDiscussionResolved = (state, getters) => discussionId => getters.resolvedDiscussionsById[discussionId] !== undefined; @@ -104,7 +115,7 @@ export const unresolvedDiscussionsIdsByDate = (state, getters) => // line numbers. export const unresolvedDiscussionsIdsByDiff = (state, getters) => getters.allResolvableDiscussions - .filter(d => !d.resolved) + .filter(d => !d.resolved && d.active) .sort((a, b) => { if (!a.diff_file || !b.diff_file) { return 0; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index bea396e5bb6..a3228f2cfea 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -22,6 +22,7 @@ export default { if (isDiscussion && isInMRPage()) { noteData.resolvable = note.resolvable; noteData.resolved = false; + noteData.active = true; noteData.resolve_path = note.resolve_path; noteData.resolve_with_issue_path = note.resolve_with_issue_path; noteData.diff_discussion = false; |