From 4bf043435906db75fa9543db96991b8cc7171f46 Mon Sep 17 00:00:00 2001 From: Samantha Ming Date: Fri, 28 Jun 2019 07:32:03 +0000 Subject: [PATCH] Rearrange discussion resolve buttons & update button display - next-discuss button is always show unless there is only 1 discussion - regroup buttons arrangements --- .../notes/components/discussion_actions.vue | 31 ++++++++++++------- .../notes/components/noteable_discussion.vue | 5 +-- .../javascripts/notes/stores/getters.js | 6 ++-- app/assets/stylesheets/pages/notes.scss | 4 +++ ...8689-regroup-jump-button-in-discussion.yml | 6 ++++ ...diff_notes_and_discussions_resolve_spec.rb | 4 +-- spec/javascripts/notes/stores/getters_spec.js | 20 ++++++++++++ 7 files changed, 54 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/58689-regroup-jump-button-in-discussion.yml 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 { 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]);