diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index 7ff623a1adcde20ed0e858a623e1293d51d5ca06..7af3ad80a398ecda44cc9d560b19401142a98026 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -157,7 +157,7 @@ export const contentTop = () => { () => getOuterHeight('#js-peek'), () => getOuterHeight('.navbar-gitlab'), ({ desktop }) => { - const container = document.querySelector('.line-resolve-all-container'); + const container = document.querySelector('.discussions-counter'); let size = 0; if (!desktop && container) { diff --git a/app/assets/javascripts/mr_notes/index.js b/app/assets/javascripts/mr_notes/index.js index a1377415efe421ec5bf0667dce84189903f30371..7424c01105277d43a1562000cff444a5ed8f9b35 100644 --- a/app/assets/javascripts/mr_notes/index.js +++ b/app/assets/javascripts/mr_notes/index.js @@ -31,6 +31,8 @@ export default function initMrNotes() { const el = document.getElementById('js-vue-discussion-counter'); if (el) { + const { blocksMerge } = el.dataset; + // eslint-disable-next-line no-new new Vue({ el, @@ -40,7 +42,11 @@ export default function initMrNotes() { }, store, render(createElement) { - return createElement('discussion-counter'); + return createElement('discussion-counter', { + props: { + blocksMerge: blocksMerge === 'true', + }, + }); }, }); } diff --git a/app/assets/javascripts/notes/components/discussion_counter.vue b/app/assets/javascripts/notes/components/discussion_counter.vue index 33819c78c0f9ab1b9bb6d4b8b8257d3327982db1..717ebcbe993b2629a97e90474e6c53f2aa940ad5 100644 --- a/app/assets/javascripts/notes/components/discussion_counter.vue +++ b/app/assets/javascripts/notes/components/discussion_counter.vue @@ -1,5 +1,5 @@ <script> -import { GlTooltipDirective, GlIcon, GlButton, GlButtonGroup } from '@gitlab/ui'; +import { GlTooltipDirective, GlButton, GlButtonGroup } from '@gitlab/ui'; import { mapGetters, mapActions } from 'vuex'; import { __ } from '~/locale'; import discussionNavigation from '../mixins/discussion_navigation'; @@ -9,46 +9,41 @@ export default { GlTooltip: GlTooltipDirective, }, components: { - GlIcon, GlButton, GlButtonGroup, }, mixins: [discussionNavigation], + props: { + blocksMerge: { + type: Boolean, + required: true, + }, + }, computed: { ...mapGetters([ - 'getUserData', 'getNoteableData', 'resolvableDiscussionsCount', 'unresolvedDiscussionsCount', - 'discussions', + 'allResolvableDiscussions', ]), - isLoggedIn() { - return this.getUserData.id; - }, allResolved() { return this.unresolvedDiscussionsCount === 0; }, - resolveAllDiscussionsIssuePath() { - return this.getNoteableData.create_issue_to_resolve_discussions_path; - }, - toggeableDiscussions() { - return this.discussions.filter((discussion) => !discussion.individual_note); - }, allExpanded() { - return this.toggeableDiscussions.every((discussion) => discussion.expanded); - }, - lineResolveClass() { - return this.allResolved ? 'line-resolve-btn is-active' : 'line-resolve-text'; + return this.allResolvableDiscussions.every((discussion) => discussion.expanded); }, toggleThreadsLabel() { return this.allExpanded ? __('Collapse all threads') : __('Expand all threads'); }, + resolveAllDiscussionsIssuePath() { + return this.getNoteableData.create_issue_to_resolve_discussions_path; + }, }, methods: { ...mapActions(['setExpandDiscussions']), handleExpandDiscussions() { this.setExpandDiscussions({ - discussionIds: this.toggeableDiscussions.map((discussion) => discussion.id), + discussionIds: this.allResolvableDiscussions.map((discussion) => discussion.id), expanded: !this.allExpanded, }); }, @@ -60,20 +55,60 @@ export default { <div v-if="resolvableDiscussionsCount > 0" ref="discussionCounter" - class="line-resolve-all-container full-width-mobile gl-display-flex d-sm-flex" + class="gl-display-flex discussions-counter" > - <div class="line-resolve-all"> - <span :class="lineResolveClass"> - <template v-if="allResolved"> - <gl-icon name="check-circle-filled" /> - {{ __('All threads resolved') }} - </template> - <template v-else> - {{ n__('%d unresolved thread', '%d unresolved threads', unresolvedDiscussionsCount) }} - </template> - </span> + <div + class="gl-display-flex gl-align-items-center gl-pl-4 gl-rounded-base gl-mr-3" + :class="{ + 'gl-bg-orange-50': blocksMerge, + 'gl-bg-gray-50': !blocksMerge, + 'gl-pr-4': allResolved, + 'gl-pr-2': !allResolved, + }" + data-testid="discussions-counter-text" + > + <template v-if="allResolved"> + {{ __('All threads resolved') }} + </template> + <template v-else> + {{ n__('%d unresolved thread', '%d unresolved threads', unresolvedDiscussionsCount) }} + <gl-button-group class="gl-ml-3"> + <gl-button + v-gl-tooltip.hover + :title="__('Jump to previous unresolved thread')" + :aria-label="__('Jump to previous unresolved thread')" + class="discussion-previous-btn gl-rounded-base! gl-px-2!" + data-track-action="click_button" + data-track-label="mr_previous_unresolved_thread" + data-track-property="click_previous_unresolved_thread_top" + icon="angle-up" + category="tertiary" + @click="jumpToPreviousDiscussion" + /> + <gl-button + v-gl-tooltip.hover + :title="__('Jump to next unresolved thread')" + :aria-label="__('Jump to next unresolved thread')" + class="discussion-next-btn gl-rounded-base! gl-px-2!" + data-track-action="click_button" + data-track-label="mr_next_unresolved_thread" + data-track-property="click_next_unresolved_thread_top" + icon="angle-down" + category="tertiary" + @click="jumpToNextDiscussion" + /> + </gl-button-group> + </template> </div> <gl-button-group> + <gl-button + v-gl-tooltip + :title="toggleThreadsLabel" + :aria-label="toggleThreadsLabel" + class="toggle-all-discussions-btn" + :icon="allExpanded ? 'collapse' : 'expand'" + @click="handleExpandDiscussions" + /> <gl-button v-if="resolveAllDiscussionsIssuePath && !allResolved" v-gl-tooltip @@ -83,26 +118,6 @@ export default { class="new-issue-for-discussion discussion-create-issue-btn" icon="issue-new" /> - <gl-button - v-if="isLoggedIn && !allResolved" - v-gl-tooltip - :title="__('Jump to next unresolved thread')" - :aria-label="__('Jump to next unresolved thread')" - class="discussion-next-btn" - data-track-action="click_button" - data-track-label="mr_next_unresolved_thread" - data-track-property="click_next_unresolved_thread_top" - icon="comment-next" - @click="jumpToNextDiscussion" - /> - <gl-button - v-gl-tooltip - :title="toggleThreadsLabel" - :aria-label="toggleThreadsLabel" - class="toggle-all-discussions-btn" - :icon="allExpanded ? 'angle-up' : 'angle-down'" - @click="handleExpandDiscussions" - /> </gl-button-group> </div> </template> diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index f95cff012d036ea372eefdd9f97ce75b366cb25f..c40871c858f9a722ef70d41d148f98ec8b540efb 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -294,8 +294,7 @@ $tabs-holder-z-index: 250; justify-content: space-between; @include media-breakpoint-down(xs) { - .discussion-filter-container, - .line-resolve-all-container { + .discussion-filter-container { margin-bottom: $gl-padding-4; } } diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 87ffa2a6423156f60256392b2f76baf517f607b1..7c5e34c0040056056cb38e69f43017bea9caa9d2 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -838,35 +838,6 @@ $system-note-svg-size: 16px; } } -.line-resolve-all-container { - > div { - white-space: nowrap; - } - - .btn-group .btn:first-child { - border-top-left-radius: 0; - border-bottom-left-radius: 0; - } -} - -.line-resolve-all { - vertical-align: middle; - display: inline-block; - padding: $gl-padding-8 $gl-padding-12; - background-color: $gray-light; - border: 1px solid $border-color; - border-right: 0; - border-radius: $border-radius-default; - border-top-right-radius: 0; - border-bottom-right-radius: 0; - font-size: $gl-font-size; - line-height: 1rem; - - @include media-breakpoint-down(xs) { - flex: 1; - } -} - .line-resolve-btn { position: relative; top: 0; diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml index c356b9d5f039f3b40f72499f74bf1c02aeb532b8..91f999eda36aa83d345c48491340e98be9bc15fe 100644 --- a/app/views/projects/merge_requests/show.html.haml +++ b/app/views/projects/merge_requests/show.html.haml @@ -41,7 +41,7 @@ = _("Changes") = gl_badge_tag @diffs_count, { size: :sm } .d-flex.flex-wrap.align-items-center.justify-content-lg-end - #js-vue-discussion-counter + #js-vue-discussion-counter{ data: { blocks_merge: @project.only_allow_merge_if_all_discussions_are_resolved?.to_s } } .tab-content#diff-notes-app #js-diff-file-finder diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1e4d43ad7e6ef1248fc6b0d1d9793b149748c18c..9bc18be19b8c8b1899efbf3508234ede2a830f85 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -21891,6 +21891,9 @@ msgstr "" msgid "Jump to next unresolved thread" msgstr "" +msgid "Jump to previous unresolved thread" +msgstr "" + msgid "Jun" msgstr "" diff --git a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb index b87ac743d0284039124fb3696f474be727c97cc6..1e8b9b6b60b6e4a8eef1dbc7533b26bbb90c75b1 100644 --- a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb +++ b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb @@ -26,7 +26,7 @@ def resolve_all_discussions_link_selector(title: "") end it 'shows a button to resolve all threads by creating a new issue' do - within('.line-resolve-all-container') do + within('.discussions-counter') do expect(page).to have_selector resolve_all_discussions_link_selector( title: "Create issue to resolve all threads" ) end end diff --git a/spec/features/merge_request/batch_comments_spec.rb b/spec/features/merge_request/batch_comments_spec.rb index 2d8d4064efdb01654dc6cac8483e16face4afc56..9b54d95be6b044e95388ed1a92dbfe324639a6df 100644 --- a/spec/features/merge_request/batch_comments_spec.rb +++ b/spec/features/merge_request/batch_comments_spec.rb @@ -172,9 +172,8 @@ write_reply_to_discussion(button_text: 'Add comment now', resolve: true) - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end @@ -189,9 +188,8 @@ wait_for_requests - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end end @@ -212,9 +210,8 @@ write_reply_to_discussion(button_text: 'Add comment now', unresolve: true) - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') - expect(page).not_to have_selector('.line-resolve-btn.is-active') end end @@ -231,9 +228,8 @@ wait_for_requests - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') - expect(page).not_to have_selector('.line-resolve-btn.is-active') end end end 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 231722c166dea4f24c64c9ffd43f661c998d6a03..e09ec11f095273c4957cb81f74bb403355a021fd 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 @@ -38,7 +38,7 @@ context 'single thread' do it 'shows text with how many threads' do - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') end end @@ -55,9 +55,8 @@ expect(page).to have_selector('.btn', text: 'Unresolve thread') end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end @@ -72,9 +71,8 @@ expect(page).to have_selector('.line-resolve-btn.is-active') end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end @@ -84,7 +82,7 @@ click_button 'Unresolve thread' end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') end end @@ -155,7 +153,7 @@ wait_for_requests end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') end end @@ -167,9 +165,8 @@ wait_for_requests end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') - expect(page).not_to have_selector('.line-resolve-btn.is-active') end end @@ -184,7 +181,7 @@ wait_for_requests end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') end end @@ -196,9 +193,8 @@ find('button[data-qa-selector="resolve_discussion_button"]').click # rubocop:disable QA/SelectorUsage end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end @@ -213,14 +209,13 @@ click_button 'Add comment now' end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end it 'allows user to quickly scroll to next unresolved thread' do - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do page.find('.discussion-next-btn').click end @@ -269,7 +264,7 @@ expect(first('.line-resolve-btn')['aria-label']).to eq("Resolved by #{user.name}") end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') end end @@ -286,7 +281,7 @@ expect(button['aria-label']).to eq("Resolved by #{user.name}") end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') end end @@ -299,7 +294,7 @@ end it 'shows text with how many threads' do - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('2 unresolved threads') end end @@ -307,7 +302,7 @@ it 'allows user to mark a single note as resolved' do click_button('Resolve thread', match: :first) - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') end end @@ -317,9 +312,8 @@ btn.click end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end @@ -330,9 +324,8 @@ end end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end @@ -341,7 +334,7 @@ find('button[data-qa-selector="resolve_discussion_button"]').click # rubocop:disable QA/SelectorUsage end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do page.find('.discussion-next-btn').click end @@ -370,7 +363,7 @@ expect(page).not_to have_selector('.btn', text: 'Resolve thread') end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do page.find('.discussion-next-btn').click end @@ -386,7 +379,7 @@ context 'changes tab' do it 'shows text with how many threads' do - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') end end @@ -402,9 +395,8 @@ expect(page).to have_selector('.btn', text: 'Unresolve thread') end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end @@ -417,9 +409,8 @@ expect(page).to have_selector('.line-resolve-btn.is-active') end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end @@ -429,7 +420,7 @@ click_button 'Unresolve thread' end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') end end @@ -445,9 +436,8 @@ click_button 'Add comment now' end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end @@ -462,7 +452,7 @@ click_button 'Add comment now' end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') end end @@ -485,7 +475,7 @@ expect(page).not_to have_selector('.line-resolve-btn') end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') end end @@ -515,9 +505,8 @@ expect(page).to have_selector('.btn', text: 'Unresolve thread') end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('All threads resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') end end end @@ -534,32 +523,11 @@ expect(page).not_to have_selector('.line-resolve-btn') end - page.within '.line-resolve-all-container' do + page.within '.discussions-counter' do expect(page).to have_content('1 unresolved thread') end end end - - context 'resolved comment' do - before do - note.resolve!(user) - visit_merge_request - end - - it 'shows resolved icon' do - expect(page).to have_content 'All threads resolved' - - click_button _('Show thread') - expect(page).to have_selector('.line-resolve-btn.is-active') - end - - it 'does not allow user to click resolve button' do - expect(page).to have_selector('.line-resolve-btn.is-active') - click_button _('Show thread') - - expect(page).to have_selector('.line-resolve-btn.is-active') - end - end end def visit_merge_request(mr = nil) diff --git a/spec/frontend/notes/components/discussion_counter_spec.js b/spec/frontend/notes/components/discussion_counter_spec.js index a856d002d2e4c4641797e3aaa098a986fff294a6..b83e3b1b715ae8a5f1b7ec75c544741fab857082 100644 --- a/spec/frontend/notes/components/discussion_counter_spec.js +++ b/spec/frontend/notes/components/discussion_counter_spec.js @@ -45,7 +45,7 @@ describe('DiscussionCounter component', () => { describe('has no discussions', () => { it('does not render', () => { - wrapper = shallowMount(DiscussionCounter, { store }); + wrapper = shallowMount(DiscussionCounter, { store, propsData: { blocksMerge: true } }); expect(wrapper.find({ ref: 'discussionCounter' }).exists()).toBe(false); }); @@ -55,7 +55,7 @@ describe('DiscussionCounter component', () => { it('does not render', () => { store.commit(types.ADD_OR_UPDATE_DISCUSSIONS, [{ ...discussionMock, resolvable: false }]); store.dispatch('updateResolvableDiscussionsCounts'); - wrapper = shallowMount(DiscussionCounter, { store }); + wrapper = shallowMount(DiscussionCounter, { store, propsData: { blocksMerge: true } }); expect(wrapper.find({ ref: 'discussionCounter' }).exists()).toBe(false); }); @@ -75,20 +75,33 @@ describe('DiscussionCounter component', () => { it('renders', () => { updateStore(); - wrapper = shallowMount(DiscussionCounter, { store }); + wrapper = shallowMount(DiscussionCounter, { store, propsData: { blocksMerge: true } }); expect(wrapper.find({ ref: 'discussionCounter' }).exists()).toBe(true); }); it.each` - title | resolved | isActive | groupLength - ${'not allResolved'} | ${false} | ${false} | ${3} - ${'allResolved'} | ${true} | ${true} | ${1} - `('renders correctly if $title', ({ resolved, isActive, groupLength }) => { + blocksMerge | color + ${true} | ${'gl-bg-orange-50'} + ${false} | ${'gl-bg-gray-50'} + `( + 'changes background color to $color if blocksMerge is $blocksMerge', + ({ blocksMerge, color }) => { + updateStore(); + wrapper = shallowMount(DiscussionCounter, { store, propsData: { blocksMerge } }); + + expect(wrapper.find('[data-testid="discussions-counter-text"]').classes()).toContain(color); + }, + ); + + it.each` + title | resolved | groupLength + ${'not allResolved'} | ${false} | ${4} + ${'allResolved'} | ${true} | ${1} + `('renders correctly if $title', ({ resolved, groupLength }) => { updateStore({ resolvable: true, resolved }); - wrapper = shallowMount(DiscussionCounter, { store }); + wrapper = shallowMount(DiscussionCounter, { store, propsData: { blocksMerge: true } }); - expect(wrapper.find(`.is-active`).exists()).toBe(isActive); expect(wrapper.findAll(GlButton)).toHaveLength(groupLength); }); }); @@ -99,7 +112,7 @@ describe('DiscussionCounter component', () => { const discussion = { ...discussionMock, expanded }; store.commit(types.ADD_OR_UPDATE_DISCUSSIONS, [discussion]); store.dispatch('updateResolvableDiscussionsCounts'); - wrapper = shallowMount(DiscussionCounter, { store }); + wrapper = shallowMount(DiscussionCounter, { store, propsData: { blocksMerge: true } }); toggleAllButton = wrapper.find('.toggle-all-discussions-btn'); }; @@ -117,26 +130,26 @@ describe('DiscussionCounter component', () => { updateStoreWithExpanded(true); expect(wrapper.vm.allExpanded).toBe(true); - expect(toggleAllButton.props('icon')).toBe('angle-up'); + expect(toggleAllButton.props('icon')).toBe('collapse'); toggleAllButton.vm.$emit('click'); await nextTick(); expect(wrapper.vm.allExpanded).toBe(false); - expect(toggleAllButton.props('icon')).toBe('angle-down'); + expect(toggleAllButton.props('icon')).toBe('expand'); }); it('expands all discussions if collapsed', async () => { updateStoreWithExpanded(false); expect(wrapper.vm.allExpanded).toBe(false); - expect(toggleAllButton.props('icon')).toBe('angle-down'); + expect(toggleAllButton.props('icon')).toBe('expand'); toggleAllButton.vm.$emit('click'); await nextTick(); expect(wrapper.vm.allExpanded).toBe(true); - expect(toggleAllButton.props('icon')).toBe('angle-up'); + expect(toggleAllButton.props('icon')).toBe('collapse'); }); }); });