Rearrange discussion resolve buttons & update button display58689-regroup-jump-button-in-discussion

- next-discuss button is always show unless there is only 1 discussion
- regroup buttons arrangements

7 files changed, 54 insertions, 22 deletions

diff --git a/app/assets/javascripts/notes/components/discussion_actions.vue b/app/assets/javascripts/notes/components/discussion_actions.vue
index 22cca756ef6..1357a5268d6 100644
--- a/app/assets/javascripts/notes/components/discussion_actions.vue
+++ b/app/assets/javascripts/notes/components/discussion_actions.vue
@@ -39,20 +39,27 @@ export default {
 </script>
 
 <template>
-  <div class="discussion-with-resolve-btn">
+  <div class="discussion-with-resolve-btn clearfix">
     <reply-placeholder class="qa-discussion-reply" @onClick="$emit('showReplyForm')" />
-    <resolve-discussion-button
-      v-if="discussion.resolvable"
-      :is-resolving="isResolving"
-      :button-title="resolveButtonTitle"
-      @onClick="$emit('resolve')"
-    />
-    <div v-if="discussion.resolvable" class="btn-group discussion-actions ml-sm-2" role="group">
-      <resolve-with-issue-button v-if="resolveWithIssuePath" :url="resolveWithIssuePath" />
-      <jump-to-next-discussion-button
-        v-if="shouldShowJumpToNextDiscussion"
-        @onClick="$emit('jumpToNextDiscussion')"
+
+    <div class="btn-group discussion-actions" role="group">
+      <resolve-discussion-button
+        v-if="discussion.resolvable"
+        :is-resolving="isResolving"
+        :button-title="resolveButtonTitle"
+        @onClick="$emit('resolve')"
+      />
+      <resolve-with-issue-button
+        v-if="discussion.resolvable && resolveWithIssuePath"
+        :url="resolveWithIssuePath"
       />
     </div>
+
+    <div
+      v-if="discussion.resolvable && shouldShowJumpToNextDiscussion"
+      class="btn-group discussion-actions ml-sm-2"
+    >
+      <jump-to-next-discussion-button @onClick="$emit('jumpToNextDiscussion')" />
+    </div>
   </div>
 </template>
diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue
index 10b15a9c38c..b8eaff32cce 100644
--- a/app/assets/javascripts/notes/components/noteable_discussion.vue
+++ b/app/assets/javascripts/notes/components/noteable_discussion.vue
@@ -126,10 +126,7 @@ export default {
       return this.discussion.resolved_by_push ? __('Automatically resolved') : __('Resolved');
     },
     shouldShowJumpToNextDiscussion() {
-      return this.showJumpToNextDiscussion(
-        this.discussion.id,
-        this.discussionsByDiffOrder ? 'diff' : 'discussion',
-      );
+      return this.showJumpToNextDiscussion(this.discussionsByDiffOrder ? 'diff' : 'discussion');
     },
     shouldRenderDiffs() {
       return this.discussion.diff_discussion && this.renderDiffFile;
diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js
index d7982be3e4b..8aa8f5037b3 100644
--- a/app/assets/javascripts/notes/stores/getters.js
+++ b/app/assets/javascripts/notes/stores/getters.js
@@ -61,15 +61,13 @@ 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') => {
+export const showJumpToNextDiscussion = (state, getters) => (mode = 'discussion') => {
   const orderedDiffs =
     mode !== 'discussion'
       ? getters.unresolvedDiscussionsIdsByDiff
       : getters.unresolvedDiscussionsIdsByDate;
 
-  const indexOf = orderedDiffs.indexOf(discussionId);
-
-  return indexOf !== -1 && indexOf < orderedDiffs.length - 1;
+  return orderedDiffs.length > 1;
 };
 
 export const isDiscussionResolved = (state, getters) => discussionId =>
diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index 824edb2869f..e880b941d67 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -657,6 +657,10 @@ $note-form-margin-left: 72px;
     margin-left: -1px;
   }
 
+  .btn-group > .discussion-create-issue-btn {
+    margin-left: -2px;
+  }
+
   svg {
     height: 15px;
   }
diff --git a/changelogs/unreleased/58689-regroup-jump-button-in-discussion.yml b/changelogs/unreleased/58689-regroup-jump-button-in-discussion.yml
new file mode 100644
index 00000000000..bf6f314f0ce
--- /dev/null
+++ b/changelogs/unreleased/58689-regroup-jump-button-in-discussion.yml
@@ -0,0 +1,6 @@
+---
+title: Improve discussion reply buttons layout and how jump to next discussion button
+  appears
+merge_request: 29779
+author:
+type: changed
diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
index 08fa4a98feb..260eec7a9ed 100644
--- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
+++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
@@ -362,14 +362,14 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
         end
       end
 
-      it 'shows jump to next discussion button except on last discussion' do
+      it 'shows jump to next discussion button on all discussions' do
         wait_for_requests
 
         all_discussion_replies = page.all('.discussion-reply-holder')
 
         expect(all_discussion_replies.count).to eq(2)
         expect(all_discussion_replies.first.all('.discussion-next-btn').count).to eq(1)
-        expect(all_discussion_replies.last.all('.discussion-next-btn').count).to eq(0)
+        expect(all_discussion_replies.last.all('.discussion-next-btn').count).to eq(1)
       end
 
       it 'displays next discussion even if hidden' do
diff --git a/spec/javascripts/notes/stores/getters_spec.js b/spec/javascripts/notes/stores/getters_spec.js
index 8f3c493dd4c..c3ed079e33b 100644
--- a/spec/javascripts/notes/stores/getters_spec.js
+++ b/spec/javascripts/notes/stores/getters_spec.js
@@ -32,6 +32,26 @@ describe('Getters Notes Store', () => {
     };
   });
 
+  describe('showJumpToNextDiscussion', () => {
+    it('should return true if there are 2 or more unresolved discussions', () => {
+      const localGetters = {
+        unresolvedDiscussionsIdsByDate: ['123', '456'],
+        allResolvableDiscussions: [],
+      };
+
+      expect(getters.showJumpToNextDiscussion(state, localGetters)()).toBe(true);
+    });
+
+    it('should return false if there are 1 or less unresolved discussions', () => {
+      const localGetters = {
+        unresolvedDiscussionsIdsByDate: ['123'],
+        allResolvableDiscussions: [],
+      };
+
+      expect(getters.showJumpToNextDiscussion(state, localGetters)()).toBe(false);
+    });
+  });
+
   describe('discussions', () => {
     it('should return all discussions in the store', () => {
       expect(getters.discussions(state)).toEqual([individualNote]);