diff --git a/app/assets/javascripts/notes/components/diff_discussion_header.vue b/app/assets/javascripts/notes/components/diff_discussion_header.vue index 29f847ae526bb486af350e28b35dbd1ad9d5740b..662e278dacfdc3d4d38aaceba1b2b644a3392e9a 100644 --- a/app/assets/javascripts/notes/components/diff_discussion_header.vue +++ b/app/assets/javascripts/notes/components/diff_discussion_header.vue @@ -9,6 +9,7 @@ import { s__, __, sprintf } from '~/locale'; import { FILE_DIFF_POSITION_TYPE } from '~/diffs/constants'; import NoteEditedText from './note_edited_text.vue'; import NoteHeader from './note_header.vue'; +import ToggleRepliesWidget from './toggle_replies_widget.vue'; export default { name: 'DiffDiscussionHeader', @@ -17,6 +18,7 @@ export default { GlAvatarLink, NoteEditedText, NoteHeader, + ToggleRepliesWidget, }, directives: { SafeHtml, @@ -95,6 +97,9 @@ export default { toggleClass() { return this.discussion.expanded ? 'expanded' : 'collapsed'; }, + replies() { + return this.notes.filter((note) => !note.system); + }, }, methods: { ...mapActions(['toggleDiscussion']), @@ -107,7 +112,7 @@ export default { <template> <div class="discussion-header gl-flex gl-items-center"> - <div v-once class="timeline-avatar gl-flex-shrink gl-shrink-0 gl-self-start"> + <div v-once class="timeline-avatar gl-shrink-0 gl-self-start"> <gl-avatar-link v-if="author" :href="author.path" @@ -119,14 +124,7 @@ export default { </gl-avatar-link> </div> <div class="timeline-content gl-ml-3 gl-w-full" :class="toggleClass"> - <note-header - :author="author" - :created-at="firstNote.created_at" - :note-id="firstNote.id" - :include-toggle="true" - :expanded="discussion.expanded" - @toggleHandler="toggleDiscussionHandler" - > + <note-header :author="author" :created-at="firstNote.created_at" :note-id="firstNote.id"> <span v-safe-html="headerText"></span> </note-header> <note-edited-text @@ -134,14 +132,13 @@ export default { :edited-at="discussion.resolved_at" :edited-by="discussion.resolved_by" :action-text="resolvedText" - class-name="discussion-headline-light js-discussion-headline gl-pl-3" + class-name="discussion-headline-light js-discussion-headline gl-mt-1 gl-pl-3" /> - <note-edited-text - v-else-if="lastUpdatedAt" - :edited-at="lastUpdatedAt" - :edited-by="lastUpdatedBy" - :action-text="__('Last updated')" - class-name="discussion-headline-light js-discussion-headline gl-pl-3" + <toggle-replies-widget + :collapsed="!discussion.expanded" + :replies="replies" + class="gl-border-t -gl-mx-3 -gl-mb-3 gl-mt-4 !gl-border-x-0 !gl-border-b-0 gl-border-t-subtle" + @toggle="toggleDiscussionHandler" /> </div> </div> diff --git a/app/assets/javascripts/notes/components/discussion_notes.vue b/app/assets/javascripts/notes/components/discussion_notes.vue index 3dd0799ca402c27350cfed9b48d4b22738c65b14..c9194734f4350b14586eeb13ac31dbf3b0c5263f 100644 --- a/app/assets/javascripts/notes/components/discussion_notes.vue +++ b/app/assets/javascripts/notes/components/discussion_notes.vue @@ -158,7 +158,7 @@ export default { :edited-at="discussion.resolved_at" :edited-by="discussion.resolved_by" :action-text="resolvedText" - class-name="discussion-headline-light js-discussion-headline discussion-resolved-text gl-mb-2 gl-ml-3" + class-name="discussion-headline-light js-discussion-headline discussion-resolved-text -gl-mt-2 gl-mb-3 gl-ml-3" /> </template> <template #avatar-badge> diff --git a/app/assets/javascripts/notes/components/note_header.vue b/app/assets/javascripts/notes/components/note_header.vue index 491ed9634801fb97e5867dd104954b602e502482..36bb5c407acf632a4c4dd826c68d5db395c88431 100644 --- a/app/assets/javascripts/notes/components/note_header.vue +++ b/app/assets/javascripts/notes/components/note_header.vue @@ -1,10 +1,10 @@ <script> -import { GlIcon, GlBadge, GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui'; +import { GlBadge, GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui'; // eslint-disable-next-line no-restricted-imports import { mapActions } from 'vuex'; import { isGid, getIdFromGraphQLId } from '~/graphql_shared/utils'; import { TYPE_ACTIVITY, TYPE_COMMENT } from '~/import/constants'; -import { __, s__ } from '~/locale'; +import { s__ } from '~/locale'; import ImportedBadge from '~/vue_shared/components/imported_badge.vue'; import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue'; @@ -12,7 +12,6 @@ export default { components: { ImportedBadge, TimeAgoTooltip, - GlIcon, GlBadge, GlLoadingIcon, }, @@ -45,16 +44,6 @@ export default { required: false, default: '', }, - includeToggle: { - type: Boolean, - required: false, - default: false, - }, - expanded: { - type: Boolean, - required: false, - default: true, - }, showSpinner: { type: Boolean, required: false, @@ -98,9 +87,6 @@ export default { authorHref() { return this.author.path || this.author.webUrl; }, - toggleChevronIconName() { - return this.expanded ? 'chevron-up' : 'chevron-down'; - }, noteTimestampLink() { if (this.noteUrl) return this.noteUrl; @@ -144,9 +130,6 @@ export default { }, methods: { ...mapActions(['setTargetNoteHash']), - handleToggle() { - this.$emit('toggleHandler'); - }, updateTargetNoteHash() { if (this.$store) { this.setTargetNoteHash(this.noteTimestampLink); @@ -161,31 +144,11 @@ export default { this.isUsernameLinkHovered = false; }, }, - i18n: { - showThread: __('Show thread'), - hideThread: __('Hide thread'), - }, }; </script> <template> <div class="note-header-info"> - <div v-if="includeToggle" ref="discussionActions" class="discussion-actions"> - <button - class="note-action-button discussion-toggle-button js-vue-toggle-button" - type="button" - data-testid="thread-toggle" - @click="handleToggle" - > - <gl-icon ref="chevronIcon" :name="toggleChevronIconName" /> - <template v-if="expanded"> - {{ $options.i18n.hideThread }} - </template> - <template v-else> - {{ $options.i18n.showThread }} - </template> - </button> - </div> <template v-if="hasAuthor"> <span v-if="emailParticipant" diff --git a/app/assets/javascripts/notes/components/toggle_replies_widget.vue b/app/assets/javascripts/notes/components/toggle_replies_widget.vue index 667cec8a4313b0d7010f2d5023fb155e1d1012d5..6c15f94ed4e6df1bf7175c575cb2094cf550bfd7 100644 --- a/app/assets/javascripts/notes/components/toggle_replies_widget.vue +++ b/app/assets/javascripts/notes/components/toggle_replies_widget.vue @@ -66,10 +66,11 @@ export default { <gl-button ref="toggle" class="gl-my-2 gl-mr-3 !gl-p-0" - :class="{ '!gl-text-blue-600': !collapsed }" + :class="{ '!gl-text-link': !collapsed }" category="tertiary" :icon="buttonIcon" :aria-label="buttonLabel" + data-testid="replies-toggle" @click="toggle" /> <template v-if="collapsed"> @@ -92,7 +93,12 @@ export default { </gl-avatar-link> </template> </gl-avatars-inline> - <gl-button class="gl-mr-2" variant="link" data-testid="expand-replies-button" @click="toggle"> + <gl-button + class="gl-mr-2 gl-self-center" + variant="link" + data-testid="expand-replies-button" + @click="toggle" + > {{ n__('%d reply', '%d replies', replies.length) }} </gl-button> <gl-sprintf :message="$options.i18n.lastReplyBy"> diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 8eed5c4a694f11d6f983a4d865519005757c1b69..5f0fac58057fa720b596a2a55d1c67530e32bb5b 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -516,6 +516,8 @@ $system-note-icon-m-left: $avatar-m-left + $icon-size-diff / $avatar-m-ratio; } &.expanded { + // Render border invisible + border-bottom: 1px solid var(--gl-background-color-subtle); border-bottom-left-radius: 0; border-bottom-right-radius: 0; } @@ -598,9 +600,6 @@ $system-note-icon-m-left: $avatar-m-left + $icon-size-diff / $avatar-m-ratio; } } - - - .user-activity-content { position: relative; diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 5667dc3f8b89f5ffdfabb60d63b6c13cc33a18aa..6006559e00c3877bb6a2714aa120bdc46f1305a3 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -27099,9 +27099,6 @@ msgstr "" msgid "Hide sidebar" msgstr "" -msgid "Hide thread" -msgstr "" - msgid "Hide tooltips or popovers" msgstr "" @@ -51180,9 +51177,6 @@ msgstr "" msgid "Show the Open list" msgstr "" -msgid "Show thread" -msgstr "" - msgid "Show whitespace changes" msgstr "" 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 713f8abb94bcef3619b3872a48ae4b39f96e5654..caaaca9f1d0882f702aa72c901c72babe7208d55 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 @@ -98,13 +98,13 @@ end it 'shows resolved thread when toggled' do - find(".timeline-content .discussion[data-discussion-id='#{note.discussion_id}'] .discussion-toggle-button").click + find(".timeline-content .discussion[data-discussion-id='#{note.discussion_id}'] [data-testid='replies-toggle']").click expect(page.find(".timeline-content #note_#{note.id}")).to be_visible end it 'renders tables in lazy-loaded resolved diff dicussions' do - find(".timeline-content .discussion[data-discussion-id='#{note.discussion_id}'] .discussion-toggle-button").click + find(".timeline-content .discussion[data-discussion-id='#{note.discussion_id}'] [data-testid='replies-toggle']").click wait_for_requests @@ -133,7 +133,7 @@ describe 'reply form' do before do - click_button _('Show thread') + click_button _('Expand replies') end it 'allows user to comment' do @@ -211,10 +211,11 @@ it 'allows user to quickly scroll to next unresolved thread', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/410109' do page.within(first('.discussions-counter')) do - page.find('.discussion-next-btn').click + click_button _('Next unresolved thread') end expect(page).to have_button('Resolve thread', visible: true) + expect(page.evaluate_script("window.pageYOffset")).to be > 0 end @@ -350,7 +351,7 @@ it 'displays next thread even if hidden' do page.all('.note-discussion', count: 2).each do |discussion| page.within discussion do - click_button _('Hide thread') + click_button text: _('Collapse replies') end end diff --git a/spec/features/merge_request/user_sees_discussions_navigation_spec.rb b/spec/features/merge_request/user_sees_discussions_navigation_spec.rb index add42914521a0d3f248879f5798533d3a1c14c0f..1c63dfde8aac56463a57eca630c71c70aa6150e0 100644 --- a/spec/features/merge_request/user_sees_discussions_navigation_spec.rb +++ b/spec/features/merge_request/user_sees_discussions_navigation_spec.rb @@ -115,7 +115,7 @@ context 'with collapsed threads' do before do page.within(first_discussion_selector) do - click_button 'Hide thread' + click_button text: 'Collapse replies' end end diff --git a/spec/frontend/notes/components/diff_discussion_header_spec.js b/spec/frontend/notes/components/diff_discussion_header_spec.js index 123d53de3f3bccee83e8cd4b92097cb9dc7afca5..e437e01d6a61c53b17d8378e4d5d132b61983413 100644 --- a/spec/frontend/notes/components/diff_discussion_header_spec.js +++ b/spec/frontend/notes/components/diff_discussion_header_spec.js @@ -1,8 +1,8 @@ -import { shallowMount } from '@vue/test-utils'; - import { nextTick } from 'vue'; import { GlAvatar, GlAvatarLink } from '@gitlab/ui'; +import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import diffDiscussionHeader from '~/notes/components/diff_discussion_header.vue'; +import ToggleRepliesWidget from '~/notes/components/toggle_replies_widget.vue'; import createStore from '~/notes/stores'; import mockDiffFile from 'jest/diffs/mock_data/diff_discussions'; @@ -13,7 +13,7 @@ describe('diff_discussion_header component', () => { let wrapper; const createComponent = ({ propsData = {} } = {}) => { - wrapper = shallowMount(diffDiscussionHeader, { + wrapper = shallowMountExtended(diffDiscussionHeader, { store, propsData: { discussion: discussionMock, @@ -166,4 +166,41 @@ describe('diff_discussion_header component', () => { }); }); }); + + describe('replies toggle', () => { + it('renders replies toggle', () => { + createComponent(); + expect(wrapper.findComponent(ToggleRepliesWidget).exists()).toBe(true); + expect(wrapper.findComponent(ToggleRepliesWidget).props()).toMatchObject({ + collapsed: false, + replies: discussionMock.notes, + }); + }); + + it('skips system notes', () => { + createComponent({ + propsData: { + discussion: { + ...discussionMock, + notes: [...discussionMock.notes, { system: true }], + }, + }, + }); + expect(wrapper.findComponent(ToggleRepliesWidget).props('replies')).toStrictEqual( + discussionMock.notes, + ); + }); + + it('passes collapsed state', () => { + createComponent({ + propsData: { + discussion: { + ...discussionMock, + expanded: false, + }, + }, + }); + expect(wrapper.findComponent(ToggleRepliesWidget).props('collapsed')).toBe(true); + }); + }); }); diff --git a/spec/frontend/notes/components/note_header_spec.js b/spec/frontend/notes/components/note_header_spec.js index f2e6a54d01baece2acc6c5dc842ffe3f2b0ec27b..291644e2732227a182f3236cef937284db57314b 100644 --- a/spec/frontend/notes/components/note_header_spec.js +++ b/spec/frontend/notes/components/note_header_spec.js @@ -14,9 +14,6 @@ const actions = { describe('NoteHeader component', () => { let wrapper; - const findActionsWrapper = () => wrapper.findComponent({ ref: 'discussionActions' }); - const findToggleThreadButton = () => wrapper.findByTestId('thread-toggle'); - const findChevronIcon = () => wrapper.findComponent({ ref: 'chevronIcon' }); const findActionText = () => wrapper.findComponent({ ref: 'actionText' }); const findTimestampLink = () => wrapper.findComponent({ ref: 'noteTimestampLink' }); const findTimestamp = () => wrapper.findComponent({ ref: 'noteTimestamp' }); @@ -61,65 +58,6 @@ describe('NoteHeader component', () => { }); }; - it('does not render discussion actions when includeToggle is false', () => { - createComponent({ - includeToggle: false, - }); - - expect(findActionsWrapper().exists()).toBe(false); - }); - - describe('when includes a toggle', () => { - it('renders discussion actions', () => { - createComponent({ - includeToggle: true, - }); - - expect(findActionsWrapper().exists()).toBe(true); - }); - - it('emits toggleHandler event on button click', () => { - createComponent({ - includeToggle: true, - }); - - wrapper.find('.note-action-button').trigger('click'); - expect(wrapper.emitted('toggleHandler')).toBeDefined(); - expect(wrapper.emitted('toggleHandler')).toHaveLength(1); - }); - - it('has chevron-up icon if expanded prop is true', () => { - createComponent({ - includeToggle: true, - expanded: true, - }); - - expect(findChevronIcon().props('name')).toBe('chevron-up'); - }); - - it('has chevron-down icon if expanded prop is false', () => { - createComponent({ - includeToggle: true, - expanded: false, - }); - - expect(findChevronIcon().props('name')).toBe('chevron-down'); - }); - - it.each` - text | expanded - ${NoteHeader.i18n.showThread} | ${false} - ${NoteHeader.i18n.hideThread} | ${true} - `('toggle button has text $text is expanded is $expanded', ({ text, expanded }) => { - createComponent({ - includeToggle: true, - expanded, - }); - - expect(findToggleThreadButton().text()).toBe(text); - }); - }); - it('renders an author link if author is passed to props', () => { createComponent({ author }); diff --git a/spec/frontend/notes/components/toggle_replies_widget_spec.js b/spec/frontend/notes/components/toggle_replies_widget_spec.js index d72fcd49ba0afa339dac302d28efbc227b159238..5d47ae1237dea53ee5a93347b398ecc5c33b1738 100644 --- a/spec/frontend/notes/components/toggle_replies_widget_spec.js +++ b/spec/frontend/notes/components/toggle_replies_widget_spec.js @@ -37,6 +37,8 @@ describe('toggle replies widget for notes', () => { it('renders collapsed state elements', () => { expect(findExpandToggleButton().exists()).toBe(true); + expect(findExpandToggleButton().props('icon')).toBe('chevron-right'); + expect(findExpandToggleButton().attributes('aria-label')).toBe('Expand replies'); expect(findAvatars().props('avatars')).toHaveLength(3); expect(findRepliesButton().exists()).toBe(true); expect(wrapper.text()).toContain('Last reply by'); @@ -64,6 +66,8 @@ describe('toggle replies widget for notes', () => { it('renders expanded state elements', () => { expect(findCollapseToggleButton().exists()).toBe(true); + expect(findCollapseToggleButton().props('icon')).toBe('chevron-down'); + expect(findCollapseToggleButton().attributes('aria-label')).toBe('Collapse replies'); }); it('emits "toggle" event when collapse toggle button is clicked', () => { diff --git a/spec/frontend/work_items/components/notes/__snapshots__/work_item_note_replying_spec.js.snap b/spec/frontend/work_items/components/notes/__snapshots__/work_item_note_replying_spec.js.snap index af930f565090ce9b83977e9d0ddeac1a8c10a4ca..796a087d869eab7a24d3ead94902b83ef1830f66 100644 --- a/spec/frontend/work_items/components/notes/__snapshots__/work_item_note_replying_spec.js.snap +++ b/spec/frontend/work_items/components/notes/__snapshots__/work_item_note_replying_spec.js.snap @@ -5,7 +5,6 @@ exports[`Work Item Note Replying should have the note body and header 1`] = ` actiontext="" author="[object Object]" emailparticipant="" - expanded="true" noteabletype="" noteurl="" showspinner="true"