From ffbba55bc822c6cd019ff6b1495335cc1426def0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 4 Aug 2016 18:01:32 -0700 Subject: Implement Jump behavior for Changes tab --- .../components/jump_to_discussion.js.es6 | 146 +++++++++++++++++---- .../diff_notes/components/resolve_count.js.es6 | 2 +- .../diff_notes/mixins/discussion.js.es6 | 3 +- .../javascripts/diff_notes/services/resolve.js.es6 | 4 +- app/assets/javascripts/merge_request_tabs.js | 1 + app/views/projects/merge_requests/_show.html.haml | 6 +- 6 files changed, 131 insertions(+), 31 deletions(-) diff --git a/app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6 b/app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6 index 83417a2703e..a2998add26c 100644 --- a/app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6 +++ b/app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6 @@ -43,37 +43,137 @@ }, methods: { jumpToNextUnresolvedDiscussion: function () { - let unresolvedIds = CommentsStore.unresolvedDiscussionIds(), - nextUnresolvedDiscussionId; - const activePage = $('.merge-request-tabs .active a').attr('data-action'), - $diffDiscussions = $('.discussion').filter(function () { - return unresolvedIds.indexOf($(this).attr('data-discussion-id')) !== -1; - }); - - unresolvedIds = unresolvedIds.sort(function (a, b) { - return $diffDiscussions.index(`[data-discussion-id="${b}"]`) > $diffDiscussions.index(`[data-discussion-id="${a}"]`); - }); + let discussionsSelector, + discussionIdsInScope, + firstUnresolvedDiscussionId, + nextUnresolvedDiscussionId, + activeTab = window.mrTabs.currentAction, + hasDiscussionsToJumpTo = true, + jumpToFirstDiscussion = !this.discussionId; + + const discussionIdsForElements = function(elements) { + return elements.map(function() { + return $(this).attr('data-discussion-id'); + }).toArray(); + }; + + const discussions = this.discussions; + + if (activeTab === 'diffs') { + discussionsSelector = '.diffs .notes[data-discussion-id]'; + discussionIdsInScope = discussionIdsForElements($(discussionsSelector)); - unresolvedIds.forEach(function (discussionId, i) { - if (this.discussionId && discussionId === this.discussionId) { - nextUnresolvedDiscussionId = unresolvedIds[i + 1]; - return; + let unresolvedDiscussionCount = 0; + for (const discussionId of discussionIdsInScope) { + const discussion = discussions[discussionId]; + if (discussion && !discussion.isResolved()) { + unresolvedDiscussionCount++; + } } - }.bind(this)); - nextUnresolvedDiscussionId = nextUnresolvedDiscussionId || unresolvedIds[0]; + if (this.discussionId && !this.discussion.isResolved()) { + // If this is the last unresolved discussion on the diffs tab, + // there are no discussions to jump to. + if (unresolvedDiscussionCount === 1) { + hasDiscussionsToJumpTo = false; + } + } else { + // If there are no unresolved discussions on the diffs tab at all, + // there are no discussions to jump to. + if (unresolvedDiscussionCount === 0) { + hasDiscussionsToJumpTo = false; + } + } + } else if (activeTab !== 'notes') { + // If we are on the commits or builds tabs, + // there are no discussions to jump to. + hasDiscussionsToJumpTo = false; + } - if (nextUnresolvedDiscussionId) { - let selector = '.discussion'; + if (!hasDiscussionsToJumpTo) { + // If there are no discussions to jump to on the current page, + // switch to the notes tab and jump to the first disucssion there. + window.mrTabs.activateTab('notes'); + activeTab = 'notes'; + jumpToFirstDiscussion = true; + } - if (activePage === 'diffs' && $(`${selector}[data-discussion-id="${nextUnresolvedDiscussionId}"]`).length) { - selector = '.diffs .notes'; + if (activeTab === 'notes') { + discussionsSelector = '.discussion[data-discussion-id]'; + discussionIdsInScope = discussionIdsForElements($(discussionsSelector)); + } + + let currentDiscussionFound = false; + for (const discussionId of discussionIdsInScope) { + const discussion = discussions[discussionId]; + + if (!discussion) { + // Discussions for comments on commits in this MR don't have a resolved status. + continue; } - $.scrollTo(`${selector}[data-discussion-id="${nextUnresolvedDiscussionId}"]`, { - offset: -($('.navbar-gitlab').outerHeight() + $('.layout-nav').outerHeight()) - }); + if (!firstUnresolvedDiscussionId && !discussion.isResolved()) { + firstUnresolvedDiscussionId = discussionId; + + if (jumpToFirstDiscussion) { + break; + } + } + + if (!jumpToFirstDiscussion) { + if (currentDiscussionFound) { + if (!discussion.isResolved()) { + nextUnresolvedDiscussionId = discussionId; + break; + } + else { + continue; + } + } + + if (discussionId === this.discussionId) { + currentDiscussionFound = true; + } + } + } + + nextUnresolvedDiscussionId = nextUnresolvedDiscussionId || firstUnresolvedDiscussionId; + + if (!nextUnresolvedDiscussionId) { + return; + } + + let $target = $(`${discussionsSelector}[data-discussion-id="${nextUnresolvedDiscussionId}"]`); + + if (activeTab === 'notes') { + $target = $target.closest('.note-discussion'); + } else if (activeTab === 'diffs') { + // Resolved discussions are hidden in the diffs tab by default. + // If they are marked unresolved on the notes tab, they will still be hidden on the diffs tab. + // When jumping between unresolved discussions on the diffs tab, we show them. + $target.closest(".content").show(); + + $target = $target.closest("tr.notes_holder"); + $target.show(); + + // If we are on the diffs tab, we don't scroll to the discussion itself, but to + // 4 diff lines above it: the line the discussion was in response to + 3 context + let prevEl; + for (let i = 0; i < 4; i++) { + prevEl = $target.prev(); + + // If the discussion doesn't have 4 lines above it, we'll have to do with fewer. + if (!prevEl.hasClass("line_holder")) { + break; + } + + $target = prevEl; + } } + + $.scrollTo($target, { + offset: -($('.navbar-gitlab').outerHeight() + $('.layout-nav').outerHeight()) + }); } } }); diff --git a/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 index c69a8a4542f..9e383b14a3e 100644 --- a/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 +++ b/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 @@ -11,7 +11,7 @@ }, computed: { allResolved: function () { - return this.resolved === this.discussionCount; + return this.resolvedDiscussionCount === this.discussionCount; } } }); diff --git a/app/assets/javascripts/diff_notes/mixins/discussion.js.es6 b/app/assets/javascripts/diff_notes/mixins/discussion.js.es6 index 34f3b200e48..a05f885201d 100644 --- a/app/assets/javascripts/diff_notes/mixins/discussion.js.es6 +++ b/app/assets/javascripts/diff_notes/mixins/discussion.js.es6 @@ -4,7 +4,7 @@ discussionCount: function () { return Object.keys(this.discussions).length; }, - resolved: function () { + resolvedDiscussionCount: function () { let resolvedCount = 0; for (const discussionId in this.discussions) { @@ -19,6 +19,7 @@ }, unresolvedDiscussionCount: function () { let unresolvedCount = 0; + for (const discussionId in this.discussions) { const discussion = this.discussions[discussionId]; diff --git a/app/assets/javascripts/diff_notes/services/resolve.js.es6 b/app/assets/javascripts/diff_notes/services/resolve.js.es6 index 5e5c476b514..de771ff814b 100644 --- a/app/assets/javascripts/diff_notes/services/resolve.js.es6 +++ b/app/assets/javascripts/diff_notes/services/resolve.js.es6 @@ -11,9 +11,7 @@ prepareRequest(namespace) { this.setCSRF(); - if (Vue.http.options.root !== `/${namespace}`) { - Vue.http.options.root = `/${namespace}`; - } + Vue.http.options.root = `/${namespace}`; } resolve(namespace, noteId) { diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index cfd10922bd0..75aa6c7a491 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -81,6 +81,7 @@ if (action === 'show') { action = 'notes'; } + this.currentAction = action; new_state = this._location.pathname.replace(/\/(commits|diffs|builds)(\.html)?\/?$/, ''); if (action !== 'notes') { new_state += "/" + action; diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index 9785b5c93a7..e6ba2453ef1 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -66,12 +66,12 @@ %li#resolve-count-app.line-resolve-all-container.pull-right.prepend-top-10.hidden-xs{ "v-cloak" => true } %resolve-count{ "inline-template" => true, ":logged-out" => "#{current_user.nil?}" } .line-resolve-all{ "v-show" => "discussionCount > 0", - ":class" => "{ 'has-next-btn': !loggedOut && resolved !== discussionCount }" } + ":class" => "{ 'has-next-btn': !loggedOut && resolvedDiscussionCount !== discussionCount }" } %span.line-resolve-btn.is-disabled{ type: "button", - ":class" => "{ 'is-active': resolved === discussionCount }" } + ":class" => "{ 'is-active': resolvedDiscussionCount === discussionCount }" } = icon("check") %span.line-resolve-text - {{ resolved }}/{{ discussionCount }} {{ discussionCount | pluralize 'discussion' }} resolved + {{ resolvedDiscussionCount }}/{{ discussionCount }} {{ discussionCount | pluralize 'discussion' }} resolved = render "discussions/jump_to_next" .tab-content#diff-notes-app -- cgit v1.2.1