Skip to content

G
gitlab-ce
Commit d6391c65 authored by Phil Hughes's avatar Phil Hughes
Browse files
Options

Merge branch '32452-multiple-discussions' into 'master' 

Resolve "Multiple discussions per line in merge request diffs"

Closes #32452

See merge request gitlab-org/gitlab-ce!28748
parents fd547ee4 d4151b14
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 1162 additions and 288 deletions
app/assets/javascripts/diffs/components/diff_discussion_reply.vue 0 → 100644
View file @ d6391c65
<script>
import { mapGetters } from 'vuex';
import NoteSignedOutWidget from '~/notes/components/note_signed_out_widget.vue';
import ReplyPlaceholder from '~/notes/components/discussion_reply_placeholder.vue';
import UserAvatarLink from '~/vue_shared/components/user_avatar/user_avatar_link.vue';
export default {
name: 'DiffDiscussionReply',
components: {
NoteSignedOutWidget,
ReplyPlaceholder,
UserAvatarLink,
},
props: {
hasForm: {
type: Boolean,
required: false,
default: false,
},
renderReplyPlaceholder: {
type: Boolean,
required: true,
},
},
computed: {
...mapGetters({
currentUser: 'getUserData',
userCanReply: 'userCanReply',
}),
},
};
</script>
<template>
<div class="discussion-reply-holder d-flex clearfix">
<template v-if="userCanReply">
<slot v-if="hasForm" name="form"></slot>
<template v-else-if="renderReplyPlaceholder">
<user-avatar-link
:link-href="currentUser.path"
:img-src="currentUser.avatar_url"
:img-alt="currentUser.name"
:img-size="40"
class="d-none d-sm-block"
/>
<reply-placeholder
class="qa-discussion-reply"
:button-text="__('Start a new discussion...')"
@onClick="$emit('showNewDiscussionForm')"
/>
</template>
</template>
<note-signed-out-widget v-else />
</div>
</template>
app/assets/javascripts/diffs/components/diff_discussions.vue
View file @ d6391c65
......@@ -80,7 +80,6 @@ export default {
v-show="isExpanded(discussion)"
:discussion="discussion"
:render-diff-file="false"
:always-expanded="true"
:discussions-by-diff-order="true"
:line="line"
:help-page-path="helpPagePath"
......
app/assets/javascripts/diffs/components/diff_file_header.vue
View file @ d6391c65
......@@ -151,7 +151,11 @@ export default {
stickyMonitor(this.$refs.header, contentTop() - fileHeaderHeight - 1, false);
},
methods: {
...mapActions('diffs', ['toggleFileDiscussions', 'toggleFullDiff']),
...mapActions('diffs', [
'toggleFileDiscussions',
'toggleFileDiscussionWrappers',
'toggleFullDiff',
]),
handleToggleFile(e, checkTarget) {
if (
!checkTarget ||
......@@ -165,7 +169,7 @@ export default {
this.$emit('showForkMessage');
},
handleToggleDiscussions() {
this.toggleFileDiscussions(this.diffFile);
this.toggleFileDiscussionWrappers(this.diffFile);
},
handleFileNameClick(e) {
const isLinkToOtherPage =
......
app/assets/javascripts/diffs/components/diff_gutter_avatars.vue
View file @ d6391c65
<script>
import { mapActions } from 'vuex';
import Icon from '~/vue_shared/components/icon.vue';
import { pluralize, truncate } from '~/lib/utils/text_utility';
import UserAvatarImage from '~/vue_shared/components/user_avatar/user_avatar_image.vue';
......@@ -19,11 +18,13 @@ export default {
type: Array,
required: true,
},
discussionsExpanded: {
type: Boolean,
required: false,
default: false,
},
},
computed: {
discussionsExpanded() {
return this.discussions.every(discussion => discussion.expanded);
},
allDiscussions() {
return this.discussions.reduce((acc, note) => acc.concat(note.notes), []);
},
......@@ -45,26 +46,14 @@ export default {
},
},
methods: {
...mapActions(['toggleDiscussion']),
getTooltipText(noteData) {
let { note } = noteData;
if (note.length > LENGTH_OF_AVATAR_TOOLTIP) {
note = truncate(note, LENGTH_OF_AVATAR_TOOLTIP);
}
return `${noteData.author.name}: ${note}`;
},
toggleDiscussions() {
const forceExpanded = this.discussions.some(discussion => !discussion.expanded);
this.discussions.forEach(discussion => {
this.toggleDiscussion({
discussionId: discussion.id,
forceExpanded,
});
});
},
},
};
</script>
......@@ -76,7 +65,7 @@ export default {
type="button"
:aria-label="__('Show comments')"
class="diff-notes-collapse js-diff-comment-avatar js-diff-comment-button"
@click="toggleDiscussions"
@click="$emit('toggleLineDiscussions')"
>
<icon :size="12" name="collapse" />
</button>
......@@ -87,7 +76,7 @@ export default {
:img-src="note.author.avatar_url"
:tooltip-text="getTooltipText(note)"
class="diff-comment-avatar js-diff-comment-avatar"
@click.native="toggleDiscussions"
@click.native="$emit('toggleLineDiscussions')"
/>
<span
v-if="moreText"
......@@ -97,7 +86,7 @@ export default {
data-container="body"
data-placement="top"
role="button"
@click="toggleDiscussions"
@click="$emit('toggleLineDiscussions')"
>+{{ moreCount }}</span
>
</template>
......
app/assets/javascripts/diffs/components/diff_line_gutter_content.vue
View file @ d6391c65
......@@ -105,7 +105,13 @@ export default {
},
},
methods: {
...mapActions('diffs', ['loadMoreLines', 'showCommentForm', 'setHighlightedRow']),
...mapActions('diffs', [
'loadMoreLines',
'showCommentForm',
'setHighlightedRow',
'toggleLineDiscussions',
'toggleLineDiscussionWrappers',
]),
handleCommentButton() {
this.showCommentForm({ lineCode: this.line.line_code, fileHash: this.fileHash });
},
......@@ -184,7 +190,14 @@ export default {
@click="setHighlightedRow(lineCode)"
>
</a>
<diff-gutter-avatars v-if="shouldShowAvatarsOnGutter" :discussions="line.discussions" />
<diff-gutter-avatars
v-if="shouldShowAvatarsOnGutter"
:discussions="line.discussions"
:discussions-expanded="line.discussionsExpanded"
@toggleLineDiscussions="
toggleLineDiscussions({ lineCode, fileHash, expanded: !line.discussionsExpanded })
"
/>
</template>
</div>
</template>
app/assets/javascripts/diffs/components/inline_diff_comment_row.vue
View file @ d6391c65
<script>
import diffDiscussions from './diff_discussions.vue';
import diffLineNoteForm from './diff_line_note_form.vue';
import { mapActions } from 'vuex';
import DiffDiscussions from './diff_discussions.vue';
import DiffLineNoteForm from './diff_line_note_form.vue';
import DiffDiscussionReply from './diff_discussion_reply.vue';
export default {
components: {
diffDiscussions,
diffLineNoteForm,
DiffDiscussions,
DiffLineNoteForm,
DiffDiscussionReply,
},
props: {
line: {
......@@ -32,10 +35,12 @@ export default {
if (!this.line.discussions || !this.line.discussions.length) {
return false;
}
return this.line.discussions.every(discussion => discussion.expanded);
return this.line.discussionsExpanded;
},
},
methods: {
...mapActions('diffs', ['showCommentForm']),
},
};
</script>
......@@ -49,13 +54,22 @@ export default {
:discussions="line.discussions"
:help-page-path="helpPagePath"
/>
<diff-line-note-form
v-if="line.hasForm"
:diff-file-hash="diffFileHash"
:line="line"
:note-target-line="line"
:help-page-path="helpPagePath"
/>
<diff-discussion-reply
:has-form="line.hasForm"
:render-reply-placeholder="Boolean(line.discussions.length)"
@showNewDiscussionForm="
showCommentForm({ lineCode: line.line_code, fileHash: diffFileHash })
"
>
<template #form>
<diff-line-note-form
:diff-file-hash="diffFileHash"
:line="line"
:note-target-line="line"
:help-page-path="helpPagePath"
/>
</template>
</diff-discussion-reply>
</div>
</td>
</tr>
......
app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue
View file @ d6391c65
<script>
import diffDiscussions from './diff_discussions.vue';
import diffLineNoteForm from './diff_line_note_form.vue';
import { mapActions } from 'vuex';
import DiffDiscussions from './diff_discussions.vue';
import DiffLineNoteForm from './diff_line_note_form.vue';
import DiffDiscussionReply from './diff_discussion_reply.vue';
export default {
components: {
diffDiscussions,
diffLineNoteForm,
DiffDiscussions,
DiffLineNoteForm,
DiffDiscussionReply,
},
props: {
line: {
......@@ -29,24 +32,30 @@ export default {
computed: {
hasExpandedDiscussionOnLeft() {
return this.line.left && this.line.left.discussions.length
? this.line.left.discussions.every(discussion => discussion.expanded)
? this.line.left.discussionsExpanded
: false;
},
hasExpandedDiscussionOnRight() {
return this.line.right && this.line.right.discussions.length
? this.line.right.discussions.every(discussion => discussion.expanded)
? this.line.right.discussionsExpanded
: false;
},
hasAnyExpandedDiscussion() {
return this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight;
},
shouldRenderDiscussionsOnLeft() {
return this.line.left && this.line.left.discussions && this.hasExpandedDiscussionOnLeft;
return (
this.line.left &&
this.line.left.discussions &&
this.line.left.discussions.length &&
this.hasExpandedDiscussionOnLeft
);
},
shouldRenderDiscussionsOnRight() {
return (
this.line.right &&
this.line.right.discussions &&
this.line.right.discussions.length &&
this.hasExpandedDiscussionOnRight &&
this.line.right.type
);
......@@ -81,6 +90,22 @@ export default {
return hasCommentFormOnLeft || hasCommentFormOnRight;
},
shouldRenderReplyPlaceholderOnLeft() {
return Boolean(
this.line.left && this.line.left.discussions && this.line.left.discussions.length,
);
},
shouldRenderReplyPlaceholderOnRight() {
return Boolean(
this.line.right && this.line.right.discussions && this.line.right.discussions.length,
);
},
},
methods: {
...mapActions('diffs', ['showCommentForm']),
showNewDiscussionForm() {
this.showCommentForm({ lineCode: this.line.line_code, fileHash: this.diffFileHash });
},
},
};
</script>
......@@ -90,37 +115,49 @@ export default {
<td class="notes-content parallel old" colspan="2">
<div v-if="shouldRenderDiscussionsOnLeft" class="content">
<diff-discussions
v-if="line.left.discussions.length"
:discussions="line.left.discussions"
:line="line.left"
:help-page-path="helpPagePath"
/>
</div>
<diff-line-note-form
v-if="showLeftSideCommentForm"
:diff-file-hash="diffFileHash"
:line="line.left"
:note-target-line="line.left"
:help-page-path="helpPagePath"
line-position="left"
/>
<diff-discussion-reply
:has-form="showLeftSideCommentForm"
:render-reply-placeholder="shouldRenderReplyPlaceholderOnLeft"
@showNewDiscussionForm="showNewDiscussionForm"
>
<template #form>
<diff-line-note-form
:diff-file-hash="diffFileHash"
:line="line.left"
:note-target-line="line.left"
:help-page-path="helpPagePath"
line-position="left"
/>
</template>
</diff-discussion-reply>
</td>
<td class="notes-content parallel new" colspan="2">
<div v-if="shouldRenderDiscussionsOnRight" class="content">
<diff-discussions
v-if="line.right.discussions.length"
:discussions="line.right.discussions"
:line="line.right"
:help-page-path="helpPagePath"
/>
</div>
<diff-line-note-form
v-if="showRightSideCommentForm"
:diff-file-hash="diffFileHash"
:line="line.right"
:note-target-line="line.right"
line-position="right"
/>
<diff-discussion-reply
:has-form="showRightSideCommentForm"
:render-reply-placeholder="shouldRenderReplyPlaceholderOnRight"
@showNewDiscussionForm="showNewDiscussionForm"
>
<template #form>
<diff-line-note-form
:diff-file-hash="diffFileHash"
:line="line.right"
:note-target-line="line.right"
line-position="right"
/>
</template>
</diff-discussion-reply>
</td>
</tr>
</template>
app/assets/javascripts/diffs/store/actions.js
View file @ d6391c65
......@@ -12,6 +12,7 @@ import {
getNoteFormData,
convertExpandLines,
idleCallback,
allDiscussionWrappersExpanded,
} from './utils';
import * as types from './mutation_types';
import {
......@@ -79,6 +80,7 @@ export const assignDiscussionsToDiff = (
discussions = rootState.notes.discussions,
) => {
const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles);
const hash = getLocationHash();
discussions
.filter(discussion => discussion.diff_discussion)
......@@ -86,6 +88,7 @@ export const assignDiscussionsToDiff = (
commit(types.SET_LINE_DISCUSSIONS_FOR_FILE, {
discussion,
diffPositionByLineCode,
hash,
});
});
......@@ -99,6 +102,10 @@ export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => {
commit(types.REMOVE_LINE_DISCUSSIONS_FOR_FILE, { fileHash: file_hash, lineCode: line_code, id });
};
export const toggleLineDiscussions = ({ commit }, options) => {
commit(types.TOGGLE_LINE_DISCUSSIONS, options);
};
export const renderFileForDiscussionId = ({ commit, rootState, state }, discussionId) => {
const discussion = rootState.notes.discussions.find(d => d.id === discussionId);
......@@ -257,6 +264,31 @@ export const toggleFileDiscussions = ({ getters, dispatch }, diff) => {
});
};
export const toggleFileDiscussionWrappers = ({ commit }, diff) => {
const discussionWrappersExpanded = allDiscussionWrappersExpanded(diff);
let linesWithDiscussions;
if (diff.highlighted_diff_lines) {
linesWithDiscussions = diff.highlighted_diff_lines.filter(line => line.discussions.length);
}
if (diff.parallel_diff_lines) {
linesWithDiscussions = diff.parallel_diff_lines.filter(
line =>
(line.left && line.left.discussions.length) ||
(line.right && line.right.discussions.length),
);
}
if (linesWithDiscussions.length) {
linesWithDiscussions.forEach(line => {
commit(types.TOGGLE_LINE_DISCUSSIONS, {
fileHash: diff.file_hash,
lineCode: line.line_code,
expanded: !discussionWrappersExpanded,
});
});
}
};
export const saveDiffDiscussion = ({ state, dispatch }, { note, formData }) => {
const postData = getNoteFormData({
commit: state.commit,
......
app/assets/javascripts/diffs/store/mutation_types.js
View file @ d6391c65
......@@ -35,3 +35,5 @@ export const ADD_CURRENT_VIEW_DIFF_FILE_LINES = 'ADD_CURRENT_VIEW_DIFF_FILE_LINE
export const TOGGLE_DIFF_FILE_RENDERING_MORE = 'TOGGLE_DIFF_FILE_RENDERING_MORE';
export const SET_SHOW_SUGGEST_POPOVER = 'SET_SHOW_SUGGEST_POPOVER';
export const TOGGLE_LINE_DISCUSSIONS = 'TOGGLE_LINE_DISCUSSIONS';
app/assets/javascripts/diffs/store/mutations.js
View file @ d6391c65
......@@ -6,6 +6,7 @@ import {
addContextLines,
prepareDiffData,
isDiscussionApplicableToLine,
updateLineInFile,
} from './utils';
import * as types from './mutation_types';
......@@ -109,7 +110,7 @@ export default {
}));
},
[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode }) {
[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode, hash }) {
const { latestDiff } = state;
const discussionLineCode = discussion.line_code;
......@@ -130,13 +131,27 @@ export default {
: [],
});
const setDiscussionsExpanded = line => {
const isLineNoteTargeted = line.discussions.some(
disc => disc.notes && disc.notes.find(note => hash === `note_${note.id}`),
);
return {
...line,
discussionsExpanded:
line.discussions && line.discussions.length
? line.discussions.some(disc => !disc.resolved) || isLineNoteTargeted
: false,
};
};
state.diffFiles = state.diffFiles.map(diffFile => {
if (diffFile.file_hash === fileHash) {
const file = { ...diffFile };
if (file.highlighted_diff_lines) {
file.highlighted_diff_lines = file.highlighted_diff_lines.map(line =>
lineCheck(line) ? mapDiscussions(line) : line,
setDiscussionsExpanded(lineCheck(line) ? mapDiscussions(line) : line),
);
}
......@@ -148,8 +163,10 @@ export default {
if (left || right) {
return {
...line,
left: line.left ? mapDiscussions(line.left) : null,
right: line.right ? mapDiscussions(line.right, () => !left) : null,
left: line.left ? setDiscussionsExpanded(mapDiscussions(line.left)) : null,
right: line.right
? setDiscussionsExpanded(mapDiscussions(line.right, () => !left))
: null,
};
}
......@@ -173,32 +190,11 @@ export default {
[types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode }) {
const selectedFile = state.diffFiles.find(f => f.file_hash === fileHash);
if (selectedFile) {
if (selectedFile.parallel_diff_lines) {
const targetLine = selectedFile.parallel_diff_lines.find(
line =>
(line.left && line.left.line_code === lineCode) ||
(line.right && line.right.line_code === lineCode),
);
if (targetLine) {
const side = targetLine.left && targetLine.left.line_code === lineCode ? 'left' : 'right';
Object.assign(targetLine[side], {
discussions: targetLine[side].discussions.filter(discussion => discussion.notes.length),
});
}
}
if (selectedFile.highlighted_diff_lines) {
const targetInlineLine = selectedFile.highlighted_diff_lines.find(
line => line.line_code === lineCode,
);
if (targetInlineLine) {
Object.assign(targetInlineLine, {
discussions: targetInlineLine.discussions.filter(discussion => discussion.notes.length),
});
}
}
updateLineInFile(selectedFile, lineCode, line =>
Object.assign(line, {
discussions: line.discussions.filter(discussion => discussion.notes.length),
}),
);
if (selectedFile.discussions && selectedFile.discussions.length) {
selectedFile.discussions = selectedFile.discussions.filter(
......@@ -207,6 +203,15 @@ export default {
}
}
},
[types.TOGGLE_LINE_DISCUSSIONS](state, { fileHash, lineCode, expanded }) {
const selectedFile = state.diffFiles.find(f => f.file_hash === fileHash);
updateLineInFile(selectedFile, lineCode, line =>
Object.assign(line, { discussionsExpanded: expanded }),
);
},
[types.TOGGLE_FOLDER_OPEN](state, path) {
state.treeEntries[path].opened = !state.treeEntries[path].opened;
},
......
app/assets/javascripts/diffs/store/utils.js
View file @ d6391c65
......@@ -454,3 +454,48 @@ export const convertExpandLines = ({
};
export const idleCallback = cb => requestIdleCallback(cb);
export const updateLineInFile = (selectedFile, lineCode, updateFn) => {
if (selectedFile.parallel_diff_lines) {
const targetLine = selectedFile.parallel_diff_lines.find(
line =>
(line.left && line.left.line_code === lineCode) ||
(line.right && line.right.line_code === lineCode),
);
if (targetLine) {
const side = targetLine.left && targetLine.left.line_code === lineCode ? 'left' : 'right';
updateFn(targetLine[side]);
}
}
if (selectedFile.highlighted_diff_lines) {
const targetInlineLine = selectedFile.highlighted_diff_lines.find(
line => line.line_code === lineCode,
);
if (targetInlineLine) {
updateFn(targetInlineLine);
}
}
};
export const allDiscussionWrappersExpanded = diff => {
const discussionsExpandedArray = [];
if (diff.parallel_diff_lines) {
diff.parallel_diff_lines.forEach(line => {
if (line.left && line.left.discussions.length) {
discussionsExpandedArray.push(line.left.discussionsExpanded);
}
if (line.right && line.right.discussions.length) {
discussionsExpandedArray.push(line.right.discussionsExpanded);
}
});
} else if (diff.highlighted_diff_lines) {
diff.parallel_diff_lines.forEach(line => {
if (line.discussions.length) {
discussionsExpandedArray.push(line.discussionsExpanded);
}
});
}
return discussionsExpandedArray.every(el => el);
};
app/assets/javascripts/notes/components/discussion_actions.vue
View file @ d6391c65
......@@ -39,15 +39,23 @@ export default {
</script>
<template>
<div class="discussion-with-resolve-btn clearfix">
<reply-placeholder class="qa-discussion-reply" @onClick="$emit('showReplyForm')" />
<div class="btn-group discussion-actions" role="group">
<resolve-discussion-button
v-if="discussion.resolvable"
:is-resolving="isResolving"
:button-title="resolveButtonTitle"
@onClick="$emit('resolve')"
<div class="discussion-with-resolve-btn">
<reply-placeholder
:button-text="s__('MergeRequests|Reply...')"
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')"
/>
<resolve-with-issue-button
v-if="discussion.resolvable && resolveWithIssuePath"
......
app/assets/javascripts/notes/components/discussion_notes.vue
View file @ d6391c65
<script>
import { mapGetters } from 'vuex';
import { mapGetters, mapActions } from 'vuex';
import { SYSTEM_NOTE } from '../constants';
import { __ } from '~/locale';
import NoteableNote from './noteable_note.vue';
import PlaceholderNote from '../../vue_shared/components/notes/placeholder_note.vue';
import PlaceholderSystemNote from '../../vue_shared/components/notes/placeholder_system_note.vue';
import PlaceholderNote from '~/vue_shared/components/notes/placeholder_note.vue';
import PlaceholderSystemNote from '~/vue_shared/components/notes/placeholder_system_note.vue';
import SystemNote from '~/vue_shared/components/notes/system_note.vue';
import NoteableNote from './noteable_note.vue';
import ToggleRepliesWidget from './toggle_replies_widget.vue';
import NoteEditedText from './note_edited_text.vue';
......@@ -72,6 +72,7 @@ export default {
},
},
methods: {
...mapActions(['toggleDiscussion']),
componentName(note) {
if (note.isPlaceholderNote) {
if (note.placeholderType === SYSTEM_NOTE) {
......@@ -101,7 +102,7 @@ export default {
<component
:is="componentName(firstNote)"
:note="componentData(firstNote)"
:line="line"
:line="line || diffLine"
:commit="commit"
:help-page-path="helpPagePath"
:show-reply-button="userCanReply"
......@@ -118,23 +119,29 @@ export default {
/>
<slot slot="avatar-badge" name="avatar-badge"></slot>
</component>
<toggle-replies-widget
v-if="hasReplies"
:collapsed="!isExpanded"
:replies="replies"
@toggle="$emit('toggleDiscussion')"
/>
<template v-if="isExpanded">
<component
:is="componentName(note)"
v-for="note in replies"
:key="note.id"
:note="componentData(note)"
:help-page-path="helpPagePath"
:line="line"
@handleDeleteNote="$emit('deleteNote')"
<div
:class="discussion.diff_discussion ? 'discussion-collapsible bordered-box clearfix' : ''"
>
<toggle-replies-widget
v-if="hasReplies"
:collapsed="!isExpanded"
:replies="replies"
:class="{ 'discussion-toggle-replies': discussion.diff_discussion }"
@toggle="toggleDiscussion({ discussionId: discussion.id })"
/>
</template>
<template v-if="isExpanded">
<component
:is="componentName(note)"
v-for="note in replies"
:key="note.id"
:note="componentData(note)"
:help-page-path="helpPagePath"
:line="line"
@handleDeleteNote="$emit('deleteNote')"
/>
</template>
<slot :show-replies="isExpanded || !hasReplies" name="footer"></slot>
</div>
</template>
<template v-else>
<component
......@@ -148,8 +155,8 @@ export default {
>
<slot v-if="index === 0" slot="avatar-badge" name="avatar-badge"></slot>
</component>
<slot :show-replies="isExpanded || !hasReplies" name="footer"></slot>
</template>
</ul>
<slot :show-replies="isExpanded || !hasReplies" name="footer"></slot>
</div>
</template>
app/assets/javascripts/notes/components/discussion_reply_placeholder.vue
View file @ d6391c65
<script>
export default {
name: 'ReplyPlaceholder',
props: {
buttonText: {
type: String,
required: true,
},
},
};
</script>
......@@ -12,6 +18,6 @@ export default {
:title="s__('MergeRequests|Add a reply')"
@click="$emit('onClick')"
>
{{ s__('MergeRequests|Reply...') }}
{{ buttonText }}
</button>
</template>
app/assets/javascripts/notes/components/noteable_discussion.vue
View file @ d6391c65
......@@ -132,7 +132,7 @@ export default {
return this.discussion.diff_discussion && this.renderDiffFile;
},
shouldGroupReplies() {
return !this.shouldRenderDiffs && !this.discussion.diff_discussion;
return !this.shouldRenderDiffs;
},
wrapperComponent() {
return this.shouldRenderDiffs ? diffWithNote : 'div';
......@@ -250,6 +250,11 @@ export default {
clearDraft(this.autosaveKey);
},
saveReply(noteText, form, callback) {
if (!noteText) {
this.cancelReplyForm();
callback();
return;
}
const postData = {
in_reply_to_discussion_id: this.discussion.reply_id,
target_type: this.getNoteableData.targetType,
......@@ -363,7 +368,6 @@ Please check your network connection and try again.`;
:line="line"
:should-group-replies="shouldGroupReplies"
@startReplying="showReplyForm"
@toggleDiscussion="toggleDiscussionHandler"
@deleteNote="deleteNoteHandler"
>
<slot slot="avatar-badge" name="avatar-badge"></slot>
......@@ -376,7 +380,7 @@ Please check your network connection and try again.`;
<div
v-else-if="showReplies"
:class="{ 'is-replying': isReplying }"
class="discussion-reply-holder"
class="discussion-reply-holder clearfix"
>
<user-avatar-link
v-if="!isReplying && userCanReply"
......
app/assets/javascripts/vue_shared/components/notes/placeholder_note.vue
View file @ d6391c65
......@@ -39,7 +39,7 @@ export default {
</script>
<template>
<timeline-entry-item class="note being-posted fade-in-half">
<timeline-entry-item class="note note-wrapper being-posted fade-in-half">
<div class="timeline-icon">
<user-avatar-link
:link-href="getUserData.path"
......
app/assets/stylesheets/pages/diff.scss
View file @ d6391c65
......@@ -1093,6 +1093,17 @@ table.code {
line-height: 0;
}
.discussion-collapsible {
margin: 0 $gl-padding $gl-padding 71px;
}
.parallel {
.discussion-collapsible {
margin: $gl-padding;
margin-top: 0;
}
}
@media (max-width: map-get($grid-breakpoints, md)-1) {
.diffs .files {
@include fixed-width-container;
......@@ -1110,6 +1121,11 @@ table.code {
padding-right: 0;
}
}
.discussion-collapsible {
margin: $gl-padding;
margin-top: 0;
}
}
.image-diff-overlay,
......
app/assets/stylesheets/pages/notes.scss
View file @ d6391c65
......@@ -134,6 +134,16 @@ $note-form-margin-left: 72px;
}
}
.discussion-toggle-replies {
border-top: 0;
border-radius: 4px 4px 0 0;
&.collapsed {
border: 0;
border-radius: 4px;
}
}
.note-created-ago,
.note-updated-at {
white-space: normal;
......@@ -462,6 +472,14 @@ $note-form-margin-left: 72px;
position: relative;
}
.notes-content .discussion-notes.diff-discussions {
border-bottom: 1px solid $border-color;
&:nth-last-child(1) {
border-bottom: 0;
}
}
.notes_holder {
font-family: $regular-font;
......@@ -517,6 +535,17 @@ $note-form-margin-left: 72px;
.discussion-reply-holder {
border-radius: 0 0 $border-radius-default $border-radius-default;
position: relative;
.discussion-form {
width: 100%;
background-color: $gray-light;
padding: 0;
}
.disabled-comment {
padding: $gl-vert-padding 0;
width: 100%;
}
}
}
......
changelogs/unreleased/32452-multiple-discussions.yml 0 → 100644
View file @ d6391c65
---
title: Resolve Multiple discussions per line in merge request diffs
merge_request: 28748
author:
type: added
changelogs/unreleased/62124-new-threaded-discussion-design.yml 0 → 100644
View file @ d6391c65
---
title: Implement borderless discussion design with new reply field
merge_request: 28580
author:
type: added
locale/gitlab.pot
View file @ d6391c65
......@@ -9827,6 +9827,9 @@ msgstr ""
msgid "Start a %{new_merge_request} with these changes"
msgstr ""
msgid "Start a new discussion..."
msgstr ""
msgid "Start a new merge request"
msgstr ""
......
spec/features/issues/create_issue_for_single_discussion_in_merge_request_spec.rb
View file @ d6391c65
......@@ -54,7 +54,7 @@ describe 'Resolve an open discussion in a merge request by creating an issue', :
context 'creating the issue' do
before do
find(resolve_discussion_selector).click
find(resolve_discussion_selector, match: :first).click
end
it 'has a hidden field for the discussion' do
......
spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
View file @ d6391c65
......@@ -368,8 +368,8 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
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(1)
expect(all_discussion_replies.first.all('.discussion-next-btn').count).to eq(2)
expect(all_discussion_replies.last.all('.discussion-next-btn').count).to eq(2)
end
it 'displays next discussion even if hidden' do
......
spec/frontend/diffs/components/diff_discussion_reply_spec.js 0 → 100644
View file @ d6391c65
import { shallowMount, createLocalVue } from '@vue/test-utils';
import Vuex from 'vuex';
import DiffDiscussionReply from '~/diffs/components/diff_discussion_reply.vue';
import ReplyPlaceholder from '~/notes/components/discussion_reply_placeholder.vue';
import NoteSignedOutWidget from '~/notes/components/note_signed_out_widget.vue';
const localVue = createLocalVue();
localVue.use(Vuex);
describe('DiffDiscussionReply', () => {
let wrapper;
let getters;
let store;
const createComponent = (props = {}, slots = {}) => {
wrapper = shallowMount(DiffDiscussionReply, {
store,
localVue,
sync: false,
propsData: {
...props,
},
slots: {
...slots,
},
});
};
afterEach(() => {
wrapper.destroy();
});
describe('if user can reply', () => {
beforeEach(() => {
getters = {
userCanReply: () => true,
getUserData: () => ({
path: 'test-path',
avatar_url: 'avatar_url',
name: 'John Doe',
}),
};
store = new Vuex.Store({
getters,
});
});
it('should render a form if component has form', () => {
createComponent(
{
renderReplyPlaceholder: false,
hasForm: true,
},
{
form: `<div id="test-form"></div>`,
},
);
expect(wrapper.find('#test-form').exists()).toBe(true);
});
it('should render a reply placeholder if there is no form', () => {
createComponent({
renderReplyPlaceholder: true,
hasForm: false,
});
expect(wrapper.find(ReplyPlaceholder).exists()).toBe(true);
});
});
it('renders a signed out widget when user is not logged in', () => {
getters = {
userCanReply: () => false,
getUserData: () => null,
};
store = new Vuex.Store({
getters,
});
createComponent({
renderReplyPlaceholder: false,
hasForm: false,
});
expect(wrapper.find(NoteSignedOutWidget).exists()).toBe(true);
});
});
spec/frontend/diffs/components/diff_gutter_avatars_spec.js 0 → 100644
View file @ d6391c65
import { shallowMount, createLocalVue } from '@vue/test-utils';
import DiffGutterAvatars from '~/diffs/components/diff_gutter_avatars.vue';
import discussionsMockData from '../mock_data/diff_discussions';
const localVue = createLocalVue();
const getDiscussionsMockData = () => [Object.assign({}, discussionsMockData)];
describe('DiffGutterAvatars', () => {
let wrapper;
const findCollapseButton = () => wrapper.find('.diff-notes-collapse');
const findMoreCount = () => wrapper.find('.diff-comments-more-count');
const findUserAvatars = () => wrapper.findAll('.diff-comment-avatar');
const createComponent = (props = {}) => {
wrapper = shallowMount(DiffGutterAvatars, {
localVue,
sync: false,
propsData: {
...props,
},
});
};
afterEach(() => {
wrapper.destroy();
});
describe('when expanded', () => {
beforeEach(() => {
createComponent({
discussions: getDiscussionsMockData(),
discussionsExpanded: true,
});
});
it('renders a collapse button when discussions are expanded', () => {
expect(findCollapseButton().exists()).toBe(true);
});
it('should emit toggleDiscussions event on button click', () => {
findCollapseButton().trigger('click');
expect(wrapper.emitted().toggleLineDiscussions).toBeTruthy();
});
});
describe('when collapsed', () => {
beforeEach(() => {
createComponent({
discussions: getDiscussionsMockData(),
discussionsExpanded: false,
});
});
it('renders user avatars and moreCount text', () => {
expect(findUserAvatars().exists()).toBe(true);
expect(findMoreCount().exists()).toBe(true);
});
it('renders correct amount of user avatars', () => {
expect(findUserAvatars().length).toBe(3);
});
it('renders correct moreCount number', () => {
expect(findMoreCount().text()).toBe('+2');
});
it('should emit toggleDiscussions event on avatars click', () => {
findUserAvatars()
.at(0)
.trigger('click');
expect(wrapper.emitted().toggleLineDiscussions).toBeTruthy();
});
it('should emit toggleDiscussions event on more count text click', () => {
findMoreCount().trigger('click');
expect(wrapper.emitted().toggleLineDiscussions).toBeTruthy();
});
});
it('renders an empty more count string if there are no discussions', () => {
createComponent({
discussions: [],
discussionsExpanded: false,
});
expect(findMoreCount().exists()).toBe(false);
});
describe('tooltip text', () => {
beforeEach(() => {
createComponent({
discussions: getDiscussionsMockData(),
discussionsExpanded: false,
});
});
it('returns original comment if it is shorter than max length', () => {
const note = wrapper.vm.discussions[0].notes[0];
expect(wrapper.vm.getTooltipText(note)).toEqual('Administrator: comment 1');
});
it('returns truncated version of comment if it is longer than max length', () => {
const note = wrapper.vm.discussions[0].notes[1];
expect(wrapper.vm.getTooltipText(note)).toEqual('Fatih Acet: comment 2 is r...');
});
});
});
spec/frontend/diffs/mock_data/diff_discussions.js 0 → 100644
View file @ d6391c65
This diff is collapsed.
spec/frontend/notes/components/discussion_notes_spec.js
View file @ d6391c65
......@@ -87,7 +87,7 @@ describe('DiscussionNotes', () => {
discussion.notes[0],
];
discussion.notes = notesData;
createComponent({ discussion });
createComponent({ discussion, shouldRenderDiffs: true });
const notes = wrapper.findAll('.notes > li');
expect(notes.at(0).is(PlaceholderSystemNote)).toBe(true);
......
spec/frontend/notes/components/discussion_reply_placeholder_spec.js
View file @ d6391c65
......@@ -2,13 +2,19 @@ import ReplyPlaceholder from '~/notes/components/discussion_reply_placeholder.vu
import { shallowMount, createLocalVue } from '@vue/test-utils';
const localVue = createLocalVue();
const buttonText = 'Test Button Text';
describe('ReplyPlaceholder', () => {
let wrapper;
const findButton = () => wrapper.find({ ref: 'button' });
beforeEach(() => {
wrapper = shallowMount(ReplyPlaceholder, {
localVue,
propsData: {
buttonText,
},
});
});
......@@ -17,9 +23,7 @@ describe('ReplyPlaceholder', () => {
});
it('emits onClick even on button click', () => {
const button = wrapper.find({ ref: 'button' });
button.trigger('click');
findButton().trigger('click');
expect(wrapper.emitted()).toEqual({
onClick: [[]],
......@@ -27,8 +31,6 @@ describe('ReplyPlaceholder', () => {
});
it('should render reply button', () => {
const button = wrapper.find({ ref: 'button' });
expect(button.text()).toEqual('Reply...');
expect(findButton().text()).toEqual(buttonText);
});
});
spec/javascripts/diffs/components/diff_file_header_spec.js
View file @ d6391c65
......@@ -521,7 +521,7 @@ describe('diff_file_header', () => {
});
describe('with discussions', () => {
it('dispatches toggleFileDiscussions when user clicks on toggle discussions button', () => {
it('dispatches toggleFileDiscussionWrappers when user clicks on toggle discussions button', () => {
const propsCopy = Object.assign({}, props);
propsCopy.diffFile.submodule = false;
propsCopy.diffFile.blob = {
......@@ -552,11 +552,11 @@ describe('diff_file_header', () => {
}),
});
spyOn(vm, 'toggleFileDiscussions');
spyOn(vm, 'toggleFileDiscussionWrappers');
vm.$el.querySelector('.js-btn-vue-toggle-comments').click();
expect(vm.toggleFileDiscussions).toHaveBeenCalled();
expect(vm.toggleFileDiscussionWrappers).toHaveBeenCalled();
});
});
});
......
spec/javascripts/diffs/components/diff_gutter_avatars_spec.js deleted 100644 → 0
View file @ fd547ee4
import Vue from 'vue';
import DiffGutterAvatarsComponent from '~/diffs/components/diff_gutter_avatars.vue';
import { COUNT_OF_AVATARS_IN_GUTTER } from '~/diffs/constants';
import store from '~/mr_notes/stores';
import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
import discussionsMockData from '../mock_data/diff_discussions';
describe('DiffGutterAvatars', () => {
let component;
const getDiscussionsMockData = () => [Object.assign({}, discussionsMockData)];
beforeEach(() => {
component = createComponentWithStore(Vue.extend(DiffGutterAvatarsComponent), store, {
discussions: getDiscussionsMockData(),
}).$mount();
});
describe('computed', () => {
describe('discussionsExpanded', () => {
it('should return true when all discussions are expanded', () => {
expect(component.discussionsExpanded).toEqual(true);
});
it('should return false when all discussions are not expanded', () => {
component.discussions[0].expanded = false;
expect(component.discussionsExpanded).toEqual(false);
});
});
describe('allDiscussions', () => {
it('should return an array of notes', () => {
expect(component.allDiscussions).toEqual([...component.discussions[0].notes]);
});
});
describe('notesInGutter', () => {
it('should return a subset of discussions to show in gutter', () => {
expect(component.notesInGutter.length).toEqual(COUNT_OF_AVATARS_IN_GUTTER);
expect(component.notesInGutter[0]).toEqual({
note: component.discussions[0].notes[0].note,
author: component.discussions[0].notes[0].author,
});
});
});
describe('moreCount', () => {
it('should return count of remaining discussions from gutter', () => {
expect(component.moreCount).toEqual(2);
});
});
describe('moreText', () => {
it('should return proper text if moreCount > 0', () => {
expect(component.moreText).toEqual('2 more comments');
});
it('should return empty string if there is no discussion', () => {
component.discussions = [];
expect(component.moreText).toEqual('');
});
});
});
describe('methods', () => {
describe('getTooltipText', () => {
it('should return original comment if it is shorter than max length', () => {
const note = component.discussions[0].notes[0];
expect(component.getTooltipText(note)).toEqual('Administrator: comment 1');
});
it('should return truncated version of comment', () => {
const note = component.discussions[0].notes[1];
expect(component.getTooltipText(note)).toEqual('Fatih Acet: comment 2 is r...');
});
});
describe('toggleDiscussions', () => {
it('should toggle all discussions', () => {
expect(component.discussions[0].expanded).toEqual(true);
component.$store.dispatch('setInitialNotes', getDiscussionsMockData());
component.discussions = component.$store.state.notes.discussions;
component.toggleDiscussions();
expect(component.discussions[0].expanded).toEqual(false);
component.$store.dispatch('setInitialNotes', []);
});
it('forces expansion of all discussions', () => {
spyOn(component.$store, 'dispatch');
component.discussions[0].expanded = true;
component.discussions.push({
...component.discussions[0],
id: '123test',
expanded: false,
});
component.toggleDiscussions();
expect(component.$store.dispatch.calls.argsFor(0)).toEqual([
'toggleDiscussion',
{
discussionId: component.discussions[0].id,
forceExpanded: true,
},
]);
expect(component.$store.dispatch.calls.argsFor(1)).toEqual([
'toggleDiscussion',
{
discussionId: component.discussions[1].id,
forceExpanded: true,
},
]);
});
});
});
describe('template', () => {
const buttonSelector = '.js-diff-comment-button';
const svgSelector = `${buttonSelector} svg`;
const avatarSelector = '.js-diff-comment-avatar';
const plusCountSelector = '.js-diff-comment-plus';
it('should have button to collapse discussions when the discussions expanded', () => {
expect(component.$el.querySelector(buttonSelector)).toBeDefined();
expect(component.$el.querySelector(svgSelector)).toBeDefined();
});
it('should have user avatars when discussions collapsed', () => {
component.discussions[0].expanded = false;
Vue.nextTick(() => {
expect(component.$el.querySelector(buttonSelector)).toBeNull();
expect(component.$el.querySelectorAll(avatarSelector).length).toEqual(4);
expect(component.$el.querySelector(plusCountSelector)).toBeDefined();
expect(component.$el.querySelector(plusCountSelector).textContent).toEqual('+2');
});
});
});
});
spec/javascripts/diffs/components/inline_diff_view_spec.js
View file @ d6391c65
......@@ -36,10 +36,11 @@ describe('InlineDiffView', () => {
it('should render discussions', done => {
const el = component.$el;
component.diffLines[1].discussions = getDiscussionsMockData();
component.diffLines[1].discussionsExpanded = true;
Vue.nextTick(() => {
expect(el.querySelectorAll('.notes_holder').length).toEqual(1);
expect(el.querySelectorAll('.notes_holder .note-discussion li').length).toEqual(5);
expect(el.querySelectorAll('.notes_holder .note-discussion li').length).toEqual(6);
expect(el.innerText.indexOf('comment 5')).toBeGreaterThan(-1);
component.$store.dispatch('setInitialNotes', []);
......
spec/javascripts/diffs/store/actions_spec.js
View file @ d6391c65
......@@ -206,6 +206,7 @@ describe('DiffsStoreActions', () => {
position_type: 'text',
},
},
hash: 'diff-content-1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a',
},
},
],
......
spec/javascripts/notes/components/noteable_discussion_spec.js
View file @ d6391c65
......@@ -36,6 +36,12 @@ describe('noteable_discussion component', () => {
});
it('should render user avatar', () => {
const discussion = { ...discussionMock };
discussion.diff_file = mockDiffFile;
discussion.diff_discussion = true;
wrapper.setProps({ discussion, renderDiffFile: true });
expect(wrapper.find('.user-avatar-link').exists()).toBe(true);
});
......
spec/support/features/discussion_comments_shared_example.rb
View file @ d6391c65
......@@ -176,7 +176,7 @@ shared_examples 'discussion comments' do |resource_name|
if resource_name == 'merge request'
let(:note_id) { find("#{comments_selector} .note:first-child", match: :first)['data-note-id'] }
let(:reply_id) { find("#{comments_selector} .note:last-child", match: :first)['data-note-id'] }
let(:reply_id) { find("#{comments_selector} .note:last-of-type", match: :first)['data-note-id'] }
it 'can be replied to after resolving' do
click_button "Resolve discussion"
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7