opendev/gerrit
Code Issues Proposed changes

Enable message hyperlinking for hidden messages

Browse Source 
Previously, when given a message ID in the URL, the messages-list would
check the already-rendered messages list for that ID and exit when it
failed to find it.

The addition of show/hide for messages broke the assumption that the
already-rendered messages list directly corresponds to the total
messages list.

With this change, scrollToMessage now searches for the message ID in
_processedMessages and loads back to that message if it is not already
shown, then continues with the prior scrollTo and highlight logic.

Bug: Issue 5676
Change-Id: If8b2db1074c3ba128090e80bf89f81c5393c557a
This commit is contained in:
Kasper Nilsson 2017-03-06 11:04:51 -08:00
parent 499ed1946b
commit e91b4e4f60
2 changed files with 42 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js
View File

@ -70,7 +70,24 @@
scrollToMessage: function(messageID) {
var el = this.$$('[data-message-id="' + messageID + '"]');
if (!el) { return; }
// If the message is hidden, expand the hidden messages back to that
// point.
if (!el) {
for (var index = 0; index < this._processedMessages.length; index++) {
if (this._processedMessages[index].id === messageID) {
break;
}
}
if (index === this._processedMessages.length) { return; }
var newMessages = this._processedMessages.slice(index,
-this._visibleMessages.length);
// Add newMessages to the beginning of _visibleMessages.
this.splice.apply(this, ['_visibleMessages', 0, 0].concat(newMessages));
// Allow the dom-repeat to stamp.
Polymer.dom.flush();
el = this.$$('[data-message-id="' + messageID + '"]');
}
el.set('message.expanded', true);
var top = el.offsetTop;

28
polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.html
View File

@ -56,6 +56,7 @@ limitations under the License.
suite('gr-messages-list tests', function() {
var element;
var messages;
var sandbox;
var getMessages = function() {
return Polymer.dom(element.root).querySelectorAll('gr-message');
@ -66,12 +67,17 @@ limitations under the License.
getConfig: function() { return Promise.resolve({}); },
getLoggedIn: function() { return Promise.resolve(false); },
});
sandbox = sinon.sandbox.create();
element = fixture('basic');
messages = _.times(3, randomMessage);
element.messages = messages;
flushAsynchronousOperations();
});
teardown(function() {
sandbox.restore();
});
test('show some old messages', function() {
assert.isTrue(element.$.messageControlsContainer.hasAttribute('hidden'));
element.messages = _.times(26, randomMessage);
@ -224,8 +230,8 @@ limitations under the License.
allMessageEls[i].set('message.expanded', false);
}
var scrollToStub = sinon.stub(window, 'scrollTo');
var highlightStub = sinon.stub(element, '_highlightEl');
var scrollToStub = sandbox.stub(window, 'scrollTo');
var highlightStub = sandbox.stub(element, '_highlightEl');
element.scrollToMessage('invalid');
@ -241,9 +247,23 @@ limitations under the License.
assert.isTrue(scrollToStub.calledOnce);
assert.isTrue(highlightStub.calledOnce);
});
scrollToStub.restore();
highlightStub.restore();
test('scroll to message offscreen', function() {
var scrollToStub = sandbox.stub(window, 'scrollTo');
var highlightStub = sandbox.stub(element, '_highlightEl');
element.messages = _.times(25, randomMessage);
flushAsynchronousOperations();
assert.isFalse(scrollToStub.called);
assert.isFalse(highlightStub.called);
var messageID = element.messages[1].id;
element.scrollToMessage(messageID);
assert.isTrue(scrollToStub.calledOnce);
assert.isTrue(highlightStub.calledOnce);
assert.equal(element._visibleMessages.length, 24);
assert.isTrue(
element.$$('[data-message-id="' + messageID + '"]')._expanded);
});
test('messages', function() {