diff options
10 files changed, 94 insertions, 144 deletions
diff --git a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue index d184a76f038..0fe0007057b 100644 --- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue +++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue @@ -71,11 +71,6 @@ export default { required: false, default: false, }, - discussions: { - type: Array, - required: false, - default: () => [], - }, }, computed: { ...mapState({ @@ -83,6 +78,7 @@ export default { diffFiles: state => state.diffs.diffFiles, }), ...mapGetters(['isLoggedIn']), + ...mapGetters('diffs', ['discussionsByLineCode']), lineHref() { return this.lineCode ? `#${this.lineCode}` : '#'; }, @@ -92,19 +88,24 @@ export default { this.showCommentButton && !this.isMatchLine && !this.isContextLine && - !this.isMetaLine && - !this.hasDiscussions + !this.hasDiscussions && + !this.isMetaLine ); }, + discussions() { + return this.discussionsByLineCode[this.lineCode] || []; + }, hasDiscussions() { return this.discussions.length > 0; }, shouldShowAvatarsOnGutter() { + let render = this.hasDiscussions && this.showCommentButton; + if (!this.lineType && this.linePosition === LINE_POSITION_RIGHT) { - return false; + render = false; } - return this.hasDiscussions && this.showCommentButton; + return render; }, }, methods: { diff --git a/app/assets/javascripts/diffs/components/diff_table_cell.vue b/app/assets/javascripts/diffs/components/diff_table_cell.vue index e8e8ddc6c5e..5962f30d9bb 100644 --- a/app/assets/javascripts/diffs/components/diff_table_cell.vue +++ b/app/assets/javascripts/diffs/components/diff_table_cell.vue @@ -67,11 +67,6 @@ export default { required: false, default: false, }, - discussions: { - type: Array, - required: false, - default: () => [], - }, }, computed: { ...mapGetters(['isLoggedIn']), @@ -141,7 +136,6 @@ export default { :is-match-line="isMatchLine" :is-context-line="isContentLine" :is-meta-line="isMetaLine" - :discussions="discussions" /> </td> </template> diff --git a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue index 1b5ae5e9f75..a6f011ff31e 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue @@ -1,5 +1,5 @@ <script> -import { mapState } from 'vuex'; +import { mapState, mapGetters } from 'vuex'; import diffDiscussions from './diff_discussions.vue'; import diffLineNoteForm from './diff_line_note_form.vue'; @@ -21,16 +21,15 @@ export default { type: Number, required: true, }, - discussions: { - type: Array, - required: false, - default: () => [], - }, }, computed: { ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), + ...mapGetters('diffs', ['discussionsByLineCode']), + discussions() { + return this.discussionsByLineCode[this.line.lineCode] || []; + }, className() { return this.discussions.length ? '' : 'js-temp-notes-holder'; }, diff --git a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue index 32d65ff994f..0e306f39a9f 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue @@ -33,11 +33,6 @@ export default { required: false, default: false, }, - discussions: { - type: Array, - required: false, - default: () => [], - }, }, data() { return { @@ -94,7 +89,6 @@ export default { :is-bottom="isBottom" :is-hover="isHover" :show-comment-button="true" - :discussions="discussions" class="diff-line-num old_line" /> <diff-table-cell @@ -104,7 +98,6 @@ export default { :line-type="newLineType" :is-bottom="isBottom" :is-hover="isHover" - :discussions="discussions" class="diff-line-num new_line" /> <td diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue index 5f30cc57a59..8e491d293e5 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_view.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue @@ -20,11 +20,7 @@ export default { }, }, computed: { - ...mapGetters('diffs', [ - 'commitId', - 'shouldRenderInlineCommentRow', - 'singleDiscussionByLineCode', - ]), + ...mapGetters('diffs', ['commitId', 'discussionsByLineCode']), ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), @@ -38,7 +34,18 @@ export default { return window.gon.user_color_scheme; }, }, - methods: {}, + methods: { + shouldRenderCommentRow(line) { + if (this.diffLineCommentForms[line.lineCode]) return true; + + const lineDiscussions = this.discussionsByLineCode[line.lineCode]; + if (lineDiscussions === undefined) { + return false; + } + + return lineDiscussions.every(discussion => discussion.expanded); + }, + }, }; </script> @@ -57,15 +64,13 @@ export default { :line="line" :is-bottom="index + 1 === diffLinesLength" :key="line.lineCode" - :discussions="singleDiscussionByLineCode(line.lineCode)" /> <inline-diff-comment-row - v-if="shouldRenderInlineCommentRow(line)" + v-if="shouldRenderCommentRow(line)" :diff-file-hash="diffFile.fileHash" :line="line" :line-index="index" :key="index" - :discussions="singleDiscussionByLineCode(line.lineCode)" /> </template> </tbody> diff --git a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue index bb9a65c83fa..05e5cafc717 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue @@ -1,5 +1,5 @@ <script> -import { mapState } from 'vuex'; +import { mapState, mapGetters } from 'vuex'; import diffDiscussions from './diff_discussions.vue'; import diffLineNoteForm from './diff_line_note_form.vue'; @@ -21,51 +21,48 @@ export default { type: Number, required: true, }, - leftDiscussions: { - type: Array, - required: false, - default: () => [], - }, - rightDiscussions: { - type: Array, - required: false, - default: () => [], - }, }, computed: { ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), + ...mapGetters('diffs', ['discussionsByLineCode']), leftLineCode() { return this.line.left.lineCode; }, rightLineCode() { return this.line.right.lineCode; }, + hasDiscussion() { + const discussions = this.discussionsByLineCode; + + return discussions[this.leftLineCode] || discussions[this.rightLineCode]; + }, hasExpandedDiscussionOnLeft() { - const discussions = this.leftDiscussions; + const discussions = this.discussionsByLineCode[this.leftLineCode]; + return discussions ? discussions.every(discussion => discussion.expanded) : false; }, hasExpandedDiscussionOnRight() { - const discussions = this.rightDiscussions; + const discussions = this.discussionsByLineCode[this.rightLineCode]; + return discussions ? discussions.every(discussion => discussion.expanded) : false; }, hasAnyExpandedDiscussion() { return this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight; }, shouldRenderDiscussionsOnLeft() { - return this.leftDiscussions && this.hasExpandedDiscussionOnLeft; + return this.discussionsByLineCode[this.leftLineCode] && this.hasExpandedDiscussionOnLeft; }, shouldRenderDiscussionsOnRight() { - return this.rightDiscussions && this.hasExpandedDiscussionOnRight && this.line.right.type; - }, - showRightSideCommentForm() { - return this.line.right.type && this.diffLineCommentForms[this.rightLineCode]; + return ( + this.discussionsByLineCode[this.rightLineCode] && + this.hasExpandedDiscussionOnRight && + this.line.right.type + ); }, className() { - return this.leftDiscussions.length > 0 || this.rightDiscussions.length > 0 - ? '' - : 'js-temp-notes-holder'; + return this.hasDiscussion ? '' : 'js-temp-notes-holder'; }, }, }; @@ -83,12 +80,13 @@ export default { class="content" > <diff-discussions - v-if="leftDiscussions.length" - :discussions="leftDiscussions" + v-if="discussionsByLineCode[leftLineCode].length" + :discussions="discussionsByLineCode[leftLineCode]" /> </div> <diff-line-note-form - v-if="diffLineCommentForms[leftLineCode]" + v-if="diffLineCommentForms[leftLineCode] && + diffLineCommentForms[leftLineCode]" :diff-file-hash="diffFileHash" :line="line.left" :note-target-line="line.left" @@ -102,12 +100,13 @@ export default { class="content" > <diff-discussions - v-if="rightDiscussions.length" - :discussions="rightDiscussions" + v-if="discussionsByLineCode[rightLineCode].length" + :discussions="discussionsByLineCode[rightLineCode]" /> </div> <diff-line-note-form - v-if="showRightSideCommentForm" + v-if="diffLineCommentForms[rightLineCode] && + diffLineCommentForms[rightLineCode] && line.right.type" :diff-file-hash="diffFileHash" :line="line.right" :note-target-line="line.right" diff --git a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue index d4e54c2bd00..0031cedc68f 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue @@ -36,16 +36,6 @@ export default { required: false, default: false, }, - leftDiscussions: { - type: Array, - required: false, - default: () => [], - }, - rightDiscussions: { - type: Array, - required: false, - default: () => [], - }, }, data() { return { @@ -126,7 +116,6 @@ export default { :is-hover="isLeftHover" :show-comment-button="true" :diff-view-type="parallelDiffViewType" - :discussions="leftDiscussions" class="diff-line-num old_line" /> <td @@ -147,7 +136,6 @@ export default { :is-hover="isRightHover" :show-comment-button="true" :diff-view-type="parallelDiffViewType" - :discussions="rightDiscussions" class="diff-line-num new_line" /> <td diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue index 4d97cb6d15d..8f8d6bbc818 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue @@ -21,11 +21,7 @@ export default { }, }, computed: { - ...mapGetters('diffs', [ - 'commitId', - 'singleDiscussionByLineCode', - 'shouldRenderParallelCommentRow', - ]), + ...mapGetters('diffs', ['commitId', 'discussionsByLineCode']), ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), @@ -55,6 +51,32 @@ export default { return window.gon.user_color_scheme; }, }, + methods: { + shouldRenderCommentRow(line) { + const leftLineCode = line.left.lineCode; + const rightLineCode = line.right.lineCode; + const discussions = this.discussionsByLineCode; + const leftDiscussions = discussions[leftLineCode]; + const rightDiscussions = discussions[rightLineCode]; + const hasDiscussion = leftDiscussions || rightDiscussions; + + const hasExpandedDiscussionOnLeft = leftDiscussions + ? leftDiscussions.every(discussion => discussion.expanded) + : false; + const hasExpandedDiscussionOnRight = rightDiscussions + ? rightDiscussions.every(discussion => discussion.expanded) + : false; + + if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) { + return true; + } + + const hasCommentFormOnLeft = this.diffLineCommentForms[leftLineCode]; + const hasCommentFormOnRight = this.diffLineCommentForms[rightLineCode]; + + return hasCommentFormOnLeft || hasCommentFormOnRight; + }, + }, }; </script> @@ -75,17 +97,13 @@ export default { :line="line" :is-bottom="index + 1 === diffLinesLength" :key="index" - :left-discussions="singleDiscussionByLineCode(line.left.lineCode)" - :right-discussions="singleDiscussionByLineCode(line.right.lineCode)" /> <parallel-diff-comment-row - v-if="shouldRenderParallelCommentRow(line)" + v-if="shouldRenderCommentRow(line)" :key="`dcr-${index}`" :line="line" :diff-file-hash="diffFile.fileHash" :line-index="index" - :left-discussions="singleDiscussionByLineCode(line.left.lineCode)" - :right-discussions="singleDiscussionByLineCode(line.right.lineCode)" /> </template> </tbody> diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index c7b9b1a16e6..d3881fa1a0a 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -75,21 +75,19 @@ export const discussionsByLineCode = (state, getters, rootState, rootGetters) => const isDiffDiscussion = note.diff_discussion; const hasLineCode = note.line_code; const isResolvable = note.resolvable; + const diffRefs = diffRefsByLineCode[note.line_code]; - if (isDiffDiscussion && hasLineCode && isResolvable) { - const diffRefs = diffRefsByLineCode[note.line_code]; - if (diffRefs) { - const refs = convertObjectPropsToCamelCase(note.position.formatter); - const originalRefs = convertObjectPropsToCamelCase(note.original_position.formatter); + if (isDiffDiscussion && hasLineCode && isResolvable && diffRefs) { + const refs = convertObjectPropsToCamelCase(note.position.formatter); + const originalRefs = convertObjectPropsToCamelCase(note.original_position.formatter); - if (_.isEqual(refs, diffRefs) || _.isEqual(originalRefs, diffRefs)) { - const lineCode = note.line_code; + if (_.isEqual(refs, diffRefs) || _.isEqual(originalRefs, diffRefs)) { + const lineCode = note.line_code; - if (acc[lineCode]) { - acc[lineCode].push(note); - } else { - acc[lineCode] = [note]; - } + if (acc[lineCode]) { + acc[lineCode].push(note); + } else { + acc[lineCode] = [note]; } } } @@ -98,47 +96,6 @@ export const discussionsByLineCode = (state, getters, rootState, rootGetters) => }, {}); }; -export const singleDiscussionByLineCode = (state, getters) => lineCode => { - if (!lineCode) return []; - const discussions = getters.discussionsByLineCode; - return discussions[lineCode] || []; -}; - -export const shouldRenderParallelCommentRow = (state, getters) => line => { - const leftLineCode = line.left.lineCode; - const rightLineCode = line.right.lineCode; - const leftDiscussions = getters.singleDiscussionByLineCode(leftLineCode); - const rightDiscussions = getters.singleDiscussionByLineCode(rightLineCode); - const hasDiscussion = leftDiscussions.length || rightDiscussions.length; - - const hasExpandedDiscussionOnLeft = leftDiscussions.length - ? leftDiscussions.every(discussion => discussion.expanded) - : false; - const hasExpandedDiscussionOnRight = rightDiscussions.length - ? rightDiscussions.every(discussion => discussion.expanded) - : false; - - if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) { - return true; - } - - const hasCommentFormOnLeft = state.diffLineCommentForms[leftLineCode]; - const hasCommentFormOnRight = state.diffLineCommentForms[rightLineCode]; - - return hasCommentFormOnLeft || hasCommentFormOnRight; -}; - -export const shouldRenderInlineCommentRow = (state, getters) => line => { - if (state.diffLineCommentForms[line.lineCode]) return true; - - const lineDiscussions = getters.singleDiscussionByLineCode(line.lineCode); - if (lineDiscussions.length === 0) { - return false; - } - - return lineDiscussions.every(discussion => discussion.expanded); -}; - // prevent babel-plugin-rewire from generating an invalid default during karma∂ tests export const getDiffFileByHash = state => fileHash => state.diffFiles.find(file => file.fileHash === fileHash); diff --git a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js index bdc94131fc2..cb85d12daf2 100644 --- a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js +++ b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js @@ -50,11 +50,7 @@ describe('DiffLineGutterContent', () => { it('should return discussions for the given lineCode', () => { const { lineCode } = getDiffFileMock().highlightedDiffLines[1]; - const component = createComponent({ - lineCode, - showCommentButton: true, - discussions: getDiscussionsMockData(), - }); + const component = createComponent({ lineCode, showCommentButton: true }); setDiscussions(component); |