From 205cc4bff46e2f99dde7ed68b7c10dc4916d6e4c Mon Sep 17 00:00:00 2001
From: Sascha Eggenberger <seggenberger@gitlab.com>
Date: Mon, 9 Sep 2024 08:24:15 +0000
Subject: [PATCH] Diff threads: Align toggle function with regular threads
Changelog: changed
---
.../components/diff_discussion_header.vue | 29 ++++-----
.../notes/components/discussion_notes.vue | 2 +-
.../notes/components/note_header.vue | 41 +-----------
.../components/toggle_replies_widget.vue | 10 ++-
app/assets/stylesheets/pages/notes.scss | 5 +-
locale/gitlab.pot | 6 --
...diff_notes_and_discussions_resolve_spec.rb | 11 ++--
.../user_sees_discussions_navigation_spec.rb | 2 +-
.../components/diff_discussion_header_spec.js | 43 ++++++++++++-
.../notes/components/note_header_spec.js | 62 -------------------
.../components/toggle_replies_widget_spec.js | 4 ++
.../work_item_note_replying_spec.js.snap | 1 -
12 files changed, 77 insertions(+), 139 deletions(-)
diff --git a/app/assets/javascripts/notes/components/diff_discussion_header.vue b/app/assets/javascripts/notes/components/diff_discussion_header.vue
index 29f847ae526bb..662e278dacfdc 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 3dd0799ca402c..c9194734f4350 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 491ed9634801f..36bb5c407acf6 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 667cec8a4313b..6c15f94ed4e6d 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 8eed5c4a694f1..5f0fac58057fa 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 5667dc3f8b89f..6006559e00c38 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 713f8abb94bce..caaaca9f1d088 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 add42914521a0..1c63dfde8aac5 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 123d53de3f3bc..e437e01d6a61c 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 f2e6a54d01bae..291644e273222 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 d72fcd49ba0af..5d47ae1237dea 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 af930f565090c..796a087d869ea 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"
--
GitLab