opendev/gerrit
Code Issues Proposed changes

Wrap messages and thread lists in dom-if

Browse Source 
Moving these elements into a dom-if allows the elements to be lazily
loaded, speeding up the initial render of the change view.

Further work may need to be done with respect to performance in
rendering the thread list, pending the efficacy of this change.

Change-Id: Icd42ee7e3f7d92b3b61b9d14924e2d3849fab892
This commit is contained in:
Kasper Nilsson
2018-04-05 13:38:03 -07:00
parent f4652c1c65
commit 1be6c1d7a4
3 changed files with 83 additions and 87 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
View File

@@ -250,12 +250,8 @@ limitations under the License.
max-width: 15rem; max-width: 15rem;
--paper-tab-ink: var(--color-link); --paper-tab-ink: var(--color-link);
} }
#threadList, gr-thread-list,
#messageList { gr-messages-list {
display: none;
}
#threadList.visible,
#messageList.visible {
display: block; display: block;
} }
#includedInOverlay { #includedInOverlay {
@@ -562,24 +558,26 @@ limitations under the License.
<span>Comment Threads</span></gr-tooltip-content> <span>Comment Threads</span></gr-tooltip-content>
</paper-tab> </paper-tab>
</paper-tabs> </paper-tabs>
<gr-messages-list id="messageList" <template is="dom-if" if="[[_showMessagesView]]">
class$="hideOnMobileOverlay [[_computeShowMessages(_showMessagesView)]]" <gr-messages-list
change-num="[[_changeNum]]" class="hideOnMobileOverlay"
labels="[[_change.labels]]" change-num="[[_changeNum]]"
messages="[[_change.messages]]" labels="[[_change.labels]]"
reviewer-updates="[[_change.reviewer_updates]]" messages="[[_change.messages]]"
change-comments="[[_changeComments]]" reviewer-updates="[[_change.reviewer_updates]]"
project-name="[[_change.project]]" change-comments="[[_changeComments]]"
show-reply-buttons="[[_loggedIn]]" project-name="[[_change.project]]"
on-reply="_handleMessageReply"></gr-messages-list> show-reply-buttons="[[_loggedIn]]"
<gr-thread-list on-reply="_handleMessageReply"></gr-messages-list>
id="threadList" </template>
threads="[[_commentThreads]]" <template is="dom-if" if="[[!_showMessagesView]]">
change="[[_change]]" <gr-thread-list
change-num="[[_changeNum]]" threads="[[_commentThreads]]"
class$="[[_computeShowThreads(_showMessagesView)]]" change="[[_change]]"
logged-in="[[_loggedIn]]" change-num="[[_changeNum]]"
on-thread-list-modified="_handleReloadDiffComments"></gr-thread-list> logged-in="[[_loggedIn]]"
on-thread-list-modified="_handleReloadDiffComments"></gr-thread-list>
</template>
</div> </div>
<gr-overlay id="downloadOverlay" with-backdrop> <gr-overlay id="downloadOverlay" with-backdrop>
<gr-download-dialog <gr-download-dialog

22
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
View File

@@ -308,6 +308,14 @@
} }
}, },
get messagesList() {
return this.$$('gr-messages-list');
},
get threadList() {
return this.$$('gr-thread-list');
},
/** /**
* @param {boolean=} opt_reset * @param {boolean=} opt_reset
*/ */
@@ -341,14 +349,6 @@
this._showMessagesView = this.$.commentTabs.selected === 0; this._showMessagesView = this.$.commentTabs.selected === 0;
}, },
_computeShowMessages(showSection) {
return showSection ? 'visible' : '';
},
_computeShowThreads(showSection) {
return !showSection ? 'visible' : '';
},
_handleEditCommitMessage(e) { _handleEditCommitMessage(e) {
this._editingCommitMessage = true; this._editingCommitMessage = true;
this.$.commitMessageEditor.focusTextarea(); this.$.commitMessageEditor.focusTextarea();
@@ -706,7 +706,7 @@
_maybeScrollToMessage(hash) { _maybeScrollToMessage(hash) {
const msgPrefix = '#message-'; const msgPrefix = '#message-';
if (hash.startsWith(msgPrefix)) { if (hash.startsWith(msgPrefix)) {
this.$.messageList.scrollToMessage(hash.substr(msgPrefix.length)); this.messagesList.scrollToMessage(hash.substr(msgPrefix.length));
} }
}, },
@@ -937,7 +937,7 @@
this.modifierPressed(e)) { return; } this.modifierPressed(e)) { return; }
e.preventDefault(); e.preventDefault();
this.$.messageList.handleExpandCollapse(true); this.messagesList.handleExpandCollapse(true);
}, },
_handleZKey(e) { _handleZKey(e) {
@@ -945,7 +945,7 @@
this.modifierPressed(e)) { return; } this.modifierPressed(e)) { return; }
e.preventDefault(); e.preventDefault();
this.$.messageList.handleExpandCollapse(false); this.messagesList.handleExpandCollapse(false);
}, },
_handleCommaKey(e) { _handleCommaKey(e) {

102
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
View File

@@ -205,18 +205,24 @@ limitations under the License.
assert.isTrue(handleCollapse.called); assert.isTrue(handleCollapse.called);
}); });
test('X should expand all messages', () => { test('X should expand all messages', done => {
const handleExpand = flush(() => {
sandbox.stub(element.$.messageList, 'handleExpandCollapse'); const handleExpand = sandbox.stub(element.messagesList,
MockInteractions.pressAndReleaseKeyOn(element, 88, null, 'x'); 'handleExpandCollapse');
assert(handleExpand.calledWith(true)); MockInteractions.pressAndReleaseKeyOn(element, 88, null, 'x');
assert(handleExpand.calledWith(true));
done();
});
}); });
test('Z should collapse all messages', () => { test('Z should collapse all messages', done => {
const handleExpand = flush(() => {
sandbox.stub(element.$.messageList, 'handleExpandCollapse'); const handleExpand = sandbox.stub(element.messagesList,
MockInteractions.pressAndReleaseKeyOn(element, 90, null, 'z'); 'handleExpandCollapse');
assert(handleExpand.calledWith(false)); MockInteractions.pressAndReleaseKeyOn(element, 90, null, 'z');
assert(handleExpand.calledWith(false));
done();
});
}); });
test('shift + R should fetch and navigate to the latest patch set', test('shift + R should fetch and navigate to the latest patch set',
@@ -295,10 +301,7 @@ limitations under the License.
}; };
setup(() => { setup(() => {
reloadStub = sandbox.stub(element.$.commentAPI, 'reloadDrafts') reloadStub = sandbox.stub(element.$.commentAPI, 'reloadDrafts')
.returns(Promise.resolve({ .returns(Promise.resolve({drafts}));
drafts,
}
));
}); });
test('drafts are reloaded when reload-drafts fired', done => { test('drafts are reloaded when reload-drafts fired', done => {
@@ -327,14 +330,13 @@ limitations under the License.
test('thread list modified', () => { test('thread list modified', () => {
sandbox.spy(element, '_handleReloadDiffComments'); sandbox.spy(element, '_handleReloadDiffComments');
return element._reloadComments().then(() => { element._showMessagesView = false;
element.$.threadList.fire('thread-list-modified'); flushAsynchronousOperations();
assert.isTrue(element._handleReloadDiffComments.called);
});
});
test('thread list modified', () => {
return element._reloadComments().then(() => { return element._reloadComments().then(() => {
element.threadList.fire('thread-list-modified');
assert.isTrue(element._handleReloadDiffComments.called);
let draftStub = sinon.stub(element._changeComments, 'computeDraftCount') let draftStub = sinon.stub(element._changeComments, 'computeDraftCount')
.returns(1); .returns(1);
assert.equal(element._computeTotalCommentCounts(5, assert.equal(element._computeTotalCommentCounts(5,
@@ -395,27 +397,18 @@ limitations under the License.
// Wait for tab to get selected // Wait for tab to get selected
flush(() => { flush(() => {
assert.equal(element.$.commentTabs.selected, 0); assert.equal(element.$.commentTabs.selected, 0);
assert.notEqual(getComputedStyle(element.$.messageList).display, assert.isTrue(element._showMessagesView);
'none');
assert.equal(getComputedStyle(element.$.threadList).display, 'none');
// Switch to comment thread tab // Switch to comment thread tab
MockInteractions.tap(element.$$('paper-tab.commentThreads')); MockInteractions.tap(element.$$('paper-tab.commentThreads'));
assert.equal(element.$.commentTabs.selected, 1); assert.equal(element.$.commentTabs.selected, 1);
assert.equal(getComputedStyle(element.$.messageList).display, assert.isFalse(element._showMessagesView);
'none');
assert.notEqual(getComputedStyle(element.$.threadList).display,
'none');
// When the change is partially reloaded (ex: Shift+R), the content // When the change is partially reloaded (ex: Shift+R), the content
// is swapped out before the tab, so messages list will display even // is swapped out before the tab, so messages list will display even
// though the tab for comment threads is still temporarily selected. // though the tab for comment threads is still temporarily selected.
element._paramsChanged(element.params); element._paramsChanged(element.params);
assert.equal(element.$.commentTabs.selected, 1); assert.equal(element.$.commentTabs.selected, 1);
assert.notEqual(getComputedStyle(element.$.messageList).display, assert.isTrue(element._showMessagesView);
'none');
assert.equal(getComputedStyle(element.$.threadList).display,
'none');
done(); done();
}); });
}); });
@@ -1021,13 +1014,17 @@ limitations under the License.
}); });
test('_openReplyDialog called with `BODY` when coming from message reply' + test('_openReplyDialog called with `BODY` when coming from message reply' +
'event', () => { 'event', done => {
const openStub = sandbox.stub(element, '_openReplyDialog'); flush(() => {
element.$.messageList.fire('reply', {message: {message: 'text'}}); const openStub = sandbox.stub(element, '_openReplyDialog');
assert(openStub.lastCall.calledWithExactly( element.messagesList.fire('reply',
element.$.replyDialog.FocusTarget.BODY), {message: {message: 'text'}});
'_openReplyDialog should have been passed BODY'); assert(openStub.lastCall.calledWithExactly(
assert.equal(openStub.callCount, 1); element.$.replyDialog.FocusTarget.BODY),
'_openReplyDialog should have been passed BODY');
assert.equal(openStub.callCount, 1);
done();
});
}); });
test('reply dialog focus can be controlled', () => { test('reply dialog focus can be controlled', () => {
@@ -1437,18 +1434,20 @@ limitations under the License.
assert.equal(element._computeHeaderClass(true), 'header editMode'); assert.equal(element._computeHeaderClass(true), 'header editMode');
}); });
test('_maybeScrollToMessage', () => { test('_maybeScrollToMessage', done => {
const scrollStub = sandbox.stub(element.$.messageList, 'scrollToMessage'); flush(() => {
const scrollStub = sandbox.stub(element.messagesList,
'scrollToMessage');
element._maybeScrollToMessage(''); element._maybeScrollToMessage('');
assert.isFalse(scrollStub.called); assert.isFalse(scrollStub.called);
element._maybeScrollToMessage('message');
element._maybeScrollToMessage('message'); assert.isFalse(scrollStub.called);
assert.isFalse(scrollStub.called); element._maybeScrollToMessage('#message-TEST');
assert.isTrue(scrollStub.called);
element._maybeScrollToMessage('#message-TEST'); assert.equal(scrollStub.lastCall.args[0], 'TEST');
assert.isTrue(scrollStub.called); done();
assert.equal(scrollStub.lastCall.args[0], 'TEST'); });
}); });
test('topic update reloads related changes', () => { test('topic update reloads related changes', () => {
@@ -1602,8 +1601,7 @@ limitations under the License.
setup(() => { setup(() => {
fireEdit = () => { fireEdit = () => {
element.$.actions element.$.actions.dispatchEvent(new CustomEvent('edit-tap'));
.dispatchEvent(new CustomEvent('edit-tap', {bubbles: false}));
}; };
sandbox.stub(element.$.metadata, '_computeShowLabelStatus'); sandbox.stub(element.$.metadata, '_computeShowLabelStatus');
sandbox.stub(element.$.metadata, '_computeLabelNames'); sandbox.stub(element.$.metadata, '_computeLabelNames');