Commit 2e08b205 authored by Phil Hughes's avatar Phil Hughes

Lazy render diff line comment button

This changes the diff line comment button to only render
after the user hovers over the line for the first time.
On initial load the button doesn't exist in the DOM to save some memory,
once the user hovers over a line the button then gets added
into the DOM and on subsequent hovers it shows/hides
like previously.

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/223077
parent 29ba61a3
...@@ -4,15 +4,13 @@ import { GlIcon } from '@gitlab/ui'; ...@@ -4,15 +4,13 @@ import { GlIcon } from '@gitlab/ui';
import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils'; import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
import DiffGutterAvatars from './diff_gutter_avatars.vue'; import DiffGutterAvatars from './diff_gutter_avatars.vue';
import { import {
MATCH_LINE_TYPE,
CONTEXT_LINE_TYPE, CONTEXT_LINE_TYPE,
LINE_POSITION_RIGHT, LINE_POSITION_RIGHT,
EMPTY_CELL_TYPE, EMPTY_CELL_TYPE,
OLD_LINE_TYPE,
OLD_NO_NEW_LINE_TYPE, OLD_NO_NEW_LINE_TYPE,
OLD_LINE_TYPE,
NEW_NO_NEW_LINE_TYPE, NEW_NO_NEW_LINE_TYPE,
LINE_HOVER_CLASS_NAME, LINE_HOVER_CLASS_NAME,
LINE_UNFOLD_CLASS_NAME,
} from '../constants'; } from '../constants';
export default { export default {
...@@ -29,10 +27,6 @@ export default { ...@@ -29,10 +27,6 @@ export default {
type: String, type: String,
required: true, required: true,
}, },
contextLinesPath: {
type: String,
required: true,
},
isHighlighted: { isHighlighted: {
type: Boolean, type: Boolean,
required: true, required: true,
...@@ -52,11 +46,6 @@ export default { ...@@ -52,11 +46,6 @@ export default {
required: false, required: false,
default: '', default: '',
}, },
isContentLine: {
type: Boolean,
required: false,
default: false,
},
isBottom: { isBottom: {
type: Boolean, type: Boolean,
required: false, required: false,
...@@ -68,6 +57,11 @@ export default { ...@@ -68,6 +57,11 @@ export default {
default: false, default: false,
}, },
}, },
data() {
return {
isCommentButtonRendered: false,
};
},
computed: { computed: {
...mapGetters(['isLoggedIn']), ...mapGetters(['isLoggedIn']),
lineCode() { lineCode() {
...@@ -81,13 +75,7 @@ export default { ...@@ -81,13 +75,7 @@ export default {
return `#${this.line.line_code || ''}`; return `#${this.line.line_code || ''}`;
}, },
shouldShowCommentButton() { shouldShowCommentButton() {
return ( return this.isHover && !this.isContextLine && !this.isMetaLine && !this.hasDiscussions;
this.isHover &&
!this.isMatchLine &&
!this.isContextLine &&
!this.isMetaLine &&
!this.hasDiscussions
);
}, },
hasDiscussions() { hasDiscussions() {
return this.line.discussions && this.line.discussions.length > 0; return this.line.discussions && this.line.discussions.length > 0;
...@@ -99,6 +87,10 @@ export default { ...@@ -99,6 +87,10 @@ export default {
return this.showCommentButton && this.hasDiscussions; return this.showCommentButton && this.hasDiscussions;
}, },
shouldRenderCommentButton() { shouldRenderCommentButton() {
if (!this.isCommentButtonRendered) {
return false;
}
if (this.isLoggedIn && this.showCommentButton) { if (this.isLoggedIn && this.showCommentButton) {
const isDiffHead = parseBoolean(getParameterByName('diff_head')); const isDiffHead = parseBoolean(getParameterByName('diff_head'));
return !isDiffHead || gon.features?.mergeRefHeadComments; return !isDiffHead || gon.features?.mergeRefHeadComments;
...@@ -106,9 +98,6 @@ export default { ...@@ -106,9 +98,6 @@ export default {
return false; return false;
}, },
isMatchLine() {
return this.line.type === MATCH_LINE_TYPE;
},
isContextLine() { isContextLine() {
return this.line.type === CONTEXT_LINE_TYPE; return this.line.type === CONTEXT_LINE_TYPE;
}, },
...@@ -126,13 +115,8 @@ export default { ...@@ -126,13 +115,8 @@ export default {
type, type,
{ {
hll: this.isHighlighted, hll: this.isHighlighted,
[LINE_UNFOLD_CLASS_NAME]: this.isMatchLine,
[LINE_HOVER_CLASS_NAME]: [LINE_HOVER_CLASS_NAME]:
this.isLoggedIn && this.isLoggedIn && this.isHover && !this.isContextLine && !this.isMetaLine,
this.isHover &&
!this.isMatchLine &&
!this.isContextLine &&
!this.isMetaLine,
}, },
]; ];
}, },
...@@ -140,6 +124,17 @@ export default { ...@@ -140,6 +124,17 @@ export default {
return this.lineType === OLD_LINE_TYPE ? this.line.old_line : this.line.new_line; return this.lineType === OLD_LINE_TYPE ? this.line.old_line : this.line.new_line;
}, },
}, },
mounted() {
this.unwatchShouldShowCommentButton = this.$watch('shouldShowCommentButton', newVal => {
if (newVal) {
this.isCommentButtonRendered = true;
this.unwatchShouldShowCommentButton();
}
});
},
beforeDestroy() {
this.unwatchShouldShowCommentButton();
},
methods: { methods: {
...mapActions('diffs', ['showCommentForm', 'setHighlightedRow', 'toggleLineDiscussions']), ...mapActions('diffs', ['showCommentForm', 'setHighlightedRow', 'toggleLineDiscussions']),
handleCommentButton() { handleCommentButton() {
...@@ -151,34 +146,32 @@ export default { ...@@ -151,34 +146,32 @@ export default {
<template> <template>
<td ref="td" :class="classNameMap"> <td ref="td" :class="classNameMap">
<div> <button
<button v-if="shouldRenderCommentButton"
v-if="shouldRenderCommentButton" v-show="shouldShowCommentButton"
v-show="shouldShowCommentButton" ref="addDiffNoteButton"
ref="addDiffNoteButton" type="button"
type="button" class="add-diff-note js-add-diff-note-button qa-diff-comment"
class="add-diff-note js-add-diff-note-button qa-diff-comment" title="Add a comment to this line"
title="Add a comment to this line" @click="handleCommentButton"
@click="handleCommentButton" >
> <gl-icon :size="12" name="comment" />
<gl-icon :size="12" name="comment" /> </button>
</button> <a
<a v-if="lineNumber"
v-if="lineNumber" ref="lineNumberRef"
ref="lineNumberRef" :data-linenumber="lineNumber"
:data-linenumber="lineNumber" :href="lineHref"
:href="lineHref" @click="setHighlightedRow(lineCode)"
@click="setHighlightedRow(lineCode)" >
> </a>
</a> <diff-gutter-avatars
<diff-gutter-avatars v-if="shouldShowAvatarsOnGutter"
v-if="shouldShowAvatarsOnGutter" :discussions="line.discussions"
:discussions="line.discussions" :discussions-expanded="line.discussionsExpanded"
:discussions-expanded="line.discussionsExpanded" @toggleLineDiscussions="
@toggleLineDiscussions=" toggleLineDiscussions({ lineCode, fileHash, expanded: !line.discussionsExpanded })
toggleLineDiscussions({ lineCode, fileHash, expanded: !line.discussionsExpanded }) "
" />
/>
</div>
</td> </td>
</template> </template>
...@@ -28,10 +28,6 @@ export default { ...@@ -28,10 +28,6 @@ export default {
type: String, type: String,
required: true, required: true,
}, },
contextLinesPath: {
type: String,
required: true,
},
line: { line: {
type: Object, type: Object,
required: true, required: true,
...@@ -106,7 +102,6 @@ export default { ...@@ -106,7 +102,6 @@ export default {
> >
<diff-table-cell <diff-table-cell
:file-hash="fileHash" :file-hash="fileHash"
:context-lines-path="contextLinesPath"
:line="line" :line="line"
:line-type="oldLineType" :line-type="oldLineType"
:is-bottom="isBottom" :is-bottom="isBottom"
...@@ -117,7 +112,6 @@ export default { ...@@ -117,7 +112,6 @@ export default {
/> />
<diff-table-cell <diff-table-cell
:file-hash="fileHash" :file-hash="fileHash"
:context-lines-path="contextLinesPath"
:line="line" :line="line"
:line-type="newLineType" :line-type="newLineType"
:is-bottom="isBottom" :is-bottom="isBottom"
......
...@@ -65,7 +65,6 @@ export default { ...@@ -65,7 +65,6 @@ export default {
:key="`${line.line_code || index}`" :key="`${line.line_code || index}`"
:file-hash="diffFile.file_hash" :file-hash="diffFile.file_hash"
:file-path="diffFile.file_path" :file-path="diffFile.file_path"
:context-lines-path="diffFile.context_lines_path"
:line="line" :line="line"
:is-bottom="index + 1 === diffLinesLength" :is-bottom="index + 1 === diffLinesLength"
/> />
......
...@@ -31,10 +31,6 @@ export default { ...@@ -31,10 +31,6 @@ export default {
type: String, type: String,
required: true, required: true,
}, },
contextLinesPath: {
type: String,
required: true,
},
line: { line: {
type: Object, type: Object,
required: true, required: true,
...@@ -144,7 +140,6 @@ export default { ...@@ -144,7 +140,6 @@ export default {
<template v-if="line.left && !isMatchLineLeft"> <template v-if="line.left && !isMatchLineLeft">
<diff-table-cell <diff-table-cell
:file-hash="fileHash" :file-hash="fileHash"
:context-lines-path="contextLinesPath"
:line="line.left" :line="line.left"
:line-type="oldLineType" :line-type="oldLineType"
:is-bottom="isBottom" :is-bottom="isBottom"
...@@ -172,7 +167,6 @@ export default { ...@@ -172,7 +167,6 @@ export default {
<template v-if="line.right && !isMatchLineRight"> <template v-if="line.right && !isMatchLineRight">
<diff-table-cell <diff-table-cell
:file-hash="fileHash" :file-hash="fileHash"
:context-lines-path="contextLinesPath"
:line="line.right" :line="line.right"
:line-type="newLineType" :line-type="newLineType"
:is-bottom="isBottom" :is-bottom="isBottom"
......
...@@ -67,7 +67,6 @@ export default { ...@@ -67,7 +67,6 @@ export default {
:key="line.line_code" :key="line.line_code"
:file-hash="diffFile.file_hash" :file-hash="diffFile.file_hash"
:file-path="diffFile.file_path" :file-path="diffFile.file_path"
:context-lines-path="diffFile.context_lines_path"
:line="line" :line="line"
:is-bottom="index + 1 === diffLinesLength" :is-bottom="index + 1 === diffLinesLength"
/> />
......
...@@ -100,7 +100,11 @@ describe('DiffTableCell', () => { ...@@ -100,7 +100,11 @@ describe('DiffTableCell', () => {
setWindowLocation({ href: `${TEST_HOST}?${query}` }); setWindowLocation({ href: `${TEST_HOST}?${query}` });
createComponent({ showCommentButton }); createComponent({ showCommentButton });
expect(findNoteButton().exists()).toBe(expectation); wrapper.setData({ isCommentButtonRendered: showCommentButton });
return wrapper.vm.$nextTick().then(() => {
expect(findNoteButton().exists()).toBe(expectation);
});
}, },
); );
...@@ -108,7 +112,6 @@ describe('DiffTableCell', () => { ...@@ -108,7 +112,6 @@ describe('DiffTableCell', () => {
isHover | otherProps | discussions | expectation isHover | otherProps | discussions | expectation
${true} | ${{}} | ${[]} | ${true} ${true} | ${{}} | ${[]} | ${true}
${false} | ${{}} | ${[]} | ${false} ${false} | ${{}} | ${[]} | ${false}
${true} | ${{ line: { ...line, type: 'match' } }} | ${[]} | ${false}
${true} | ${{ line: { ...line, type: 'context' } }} | ${[]} | ${false} ${true} | ${{ line: { ...line, type: 'context' } }} | ${[]} | ${false}
${true} | ${{ line: { ...line, type: 'old-nonewline' } }} | ${[]} | ${false} ${true} | ${{ line: { ...line, type: 'old-nonewline' } }} | ${[]} | ${false}
${true} | ${{}} | ${[{}]} | ${false} ${true} | ${{}} | ${[{}]} | ${false}
...@@ -122,7 +125,13 @@ describe('DiffTableCell', () => { ...@@ -122,7 +125,13 @@ describe('DiffTableCell', () => {
...otherProps, ...otherProps,
}); });
expect(findNoteButton().isVisible()).toBe(expectation); wrapper.setData({
isCommentButtonRendered: true,
});
return wrapper.vm.$nextTick().then(() => {
expect(findNoteButton().isVisible()).toBe(expectation);
});
}, },
); );
}); });
......
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