opendev/gerrit
Code Issues Proposed changes

Handle message timestamp taps with Nav API

Browse Source 
Very similar to Ib045b8f. Removes more references to page.show, adds
'messageHash' field to getUrlForChange, and utilizes it in the change
view.

Change-Id: I496d731ceeba57843d617fd5b978cd377ea39787
This commit is contained in:
Kasper Nilsson 2018-10-19 15:11:07 -07:00
parent b28b504f8a
commit 9c1a3db8e4
11 changed files with 77 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -587,6 +587,7 @@ limitations under the License.
change-comments="[[_changeComments]]"
project-name="[[_change.project]]"
show-reply-buttons="[[_loggedIn]]"
on-message-anchor-tap="_handleMessageAnchorTap"
on-reply="_handleMessageReply"></gr-messages-list>
</template>
<template is="dom-if" if="[[!_showMessagesView]]">

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

@ -44,6 +44,8 @@
const TRAILING_WHITESPACE_REGEX = /[ \t]+$/gm;
const MSG_PREFIX = '#message-';
const ReloadToastMessage = {
NEWER_REVISION: 'A newer patch set has been uploaded',
RESTORED: 'This change has been restored',
@ -720,10 +722,17 @@
this.viewState.numFilesShown = numFilesShown;
},
_handleMessageAnchorTap(e) {
const hash = MSG_PREFIX + e.detail.id;
const url = Gerrit.Nav.getUrlForChange(this._change,
this._patchRange.patchNum, this._patchRange.basePatchNum,
this._editMode, hash);
history.replaceState(null, '', url);
},
_maybeScrollToMessage(hash) {
const msgPrefix = '#message-';
if (hash.startsWith(msgPrefix)) {
this.messagesList.scrollToMessage(hash.substr(msgPrefix.length));
if (hash.startsWith(MSG_PREFIX)) {
this.messagesList.scrollToMessage(hash.substr(MSG_PREFIX.length));
}
},

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

@ -94,6 +94,20 @@ limitations under the License.
return element.getComputedStyleValue(cssParam);
};
test('_handleMessageAnchorTap', () => {
element._changeNum = '1';
element._patchRange = {
basePatchNum: 'PARENT',
patchNum: 1,
};
const getUrlStub = sandbox.stub(Gerrit.Nav, 'getUrlForChange');
const replaceStateStub = sandbox.stub(history, 'replaceState');
element._handleMessageAnchorTap({detail: {id: 'a12345'}});
assert.equal(getUrlStub.lastCall.args[4], '#message-a12345');
assert.isTrue(replaceStateStub.called);
});
suite('keyboard shortcuts', () => {
test('S should toggle the CL star', () => {
const starStub = sandbox.stub(element.$.changeStar, 'toggleStar');

9
polygerrit-ui/app/elements/change/gr-message/gr-message.html
View File

@ -132,9 +132,12 @@ limitations under the License.
right: var(--default-horizontal-margin);
top: 10px;
}
.date {
span.date {
color: var(--deemphasized-text-color);
}
span.date:hover {
text-decoration: underline;
}
.dateContainer iron-icon {
cursor: pointer;
}
@ -227,12 +230,12 @@ limitations under the License.
</span>
</template>
<template is="dom-if" if="[[message.id]]">
<a class="date" href$="[[_computeMessageHash(message)]]" on-tap="_handleLinkTap">
<span class="date" on-tap="_handleAnchorTap">
<gr-date-formatter
has-tooltip
show-date-and-time
date-str="[[message.date]]"></gr-date-formatter>
</a>
</span>
</template>
<iron-icon
id="expandToggle"

32
polygerrit-ui/app/elements/change/gr-message/gr-message.js
View File

@ -23,18 +23,18 @@
Polymer({
is: 'gr-message',
/**
* Fired when this message's permalink is tapped.
*
* @event scroll-to
*/
/**
* Fired when this message's reply link is tapped.
*
* @event reply
*/
/**
* Fired when the message's timestamp is tapped.
*
* @event message-anchor-tap
*/
listeners: {
tap: '_handleTap',
},
@ -223,22 +223,12 @@
return classes.join(' ');
},
_computeMessageHash(message) {
return '#message-' + message.id;
},
_handleLinkTap(e) {
_handleAnchorTap(e) {
e.preventDefault();
this.fire('scroll-to', {message: this.message}, {bubbles: false});
const hash = this._computeMessageHash(this.message);
// Don't add the hash to the window history if it's already there.
// Otherwise you mess up expected back button behavior.
if (window.location.hash == hash) { return; }
// Change the URL but don’t trigger a nav event. Otherwise it will
// reload the page.
page.show(window.location.pathname + hash, null, false);
this.dispatchEvent(new CustomEvent('message-anchor-tap', {
bubbles: true,
detail: {id: this.message.id},
}));
},
_handleReplyTap(e) {

18
polygerrit-ui/app/elements/change/gr-message/gr-message_test.html
View File

@ -164,6 +164,24 @@ limitations under the License.
});
});
test('clicking on date link fires event', () => {
element.message = {
type: 'REVIEWER_UPDATE',
updated: '2016-01-12 20:24:49.448000000',
reviewer: {},
id: '47c43261_55aa2c41',
};
flushAsynchronousOperations();
const stub = sinon.stub();
element.addEventListener('message-anchor-tap', stub);
const dateEl = element.$$('.date');
assert.ok(dateEl);
MockInteractions.tap(dateEl);
assert.isTrue(stub.called);
assert.deepEqual(stub.lastCall.args[0].detail, {id: element.message.id});
});
test('votes', () => {
element.message = {
author: {},

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

@ -109,7 +109,7 @@ limitations under the License.
hide-automated="[[_hideAutomated]]"
project-name="[[projectName]]"
show-reply-button="[[showReplyButtons]]"
on-scroll-to="_handleScrollTo"
on-message-anchor-tap="_handleAnchorTap"
label-extremes="[[_labelExtremes]]"
data-message-id$="[[message.id]]"></gr-message>
</template>

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

@ -184,8 +184,8 @@
this.handleExpandCollapse(!this._expanded);
},
_handleScrollTo(e) {
this.scrollToMessage(e.detail.message.id);
_handleAnchorTap(e) {
this.scrollToMessage(e.detail.id);
},
_hasAutomatedMessages(messages) {

7
polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html
View File

@ -33,6 +33,8 @@ limitations under the License.
// also be provided.
// - `edit`, optional, Boolean: whether or not to load the file list with
// edit controls.
// - `messageHash`, optional, String: the hash of the change message to
// scroll to.
//
// - Gerrit.Nav.View.SEARCH:
// - `query`, optional, String: the literal search query. If provided,
@ -352,9 +354,11 @@ limitations under the License.
* @param {number|string=} opt_basePatchNum The string 'PARENT' can be
* used for none.
* @param {boolean=} opt_isEdit
* @param {string=} opt_messageHash
* @return {string}
*/
getUrlForChange(change, opt_patchNum, opt_basePatchNum, opt_isEdit) {
getUrlForChange(change, opt_patchNum, opt_basePatchNum, opt_isEdit,
opt_messageHash) {
if (opt_basePatchNum === PARENT_PATCHNUM) {
opt_basePatchNum = undefined;
}
@ -368,6 +372,7 @@ limitations under the License.
basePatchNum: opt_basePatchNum,
edit: opt_isEdit,
host: change.internalHost || undefined,
messageHash: opt_messageHash,
});
},

3
polygerrit-ui/app/elements/core/gr-router/gr-router.js
View File

@ -395,6 +395,9 @@
} else if (params.edit) {
suffix += ',edit';
}
if (params.messageHash) {
suffix += params.messageHash;
}
if (params.project) {
const encodedProject = this.encodeURL(params.project, true);
return `/c/${encodedProject}/+/${params.changeNum}${suffix}`;

3
polygerrit-ui/app/elements/core/gr-router/gr-router_test.html
View File

@ -281,6 +281,9 @@ limitations under the License.
paramsWithQuery.basePatchNum = 5;
assert.equal(element._generateUrl(paramsWithQuery),
'/c/test/+/1234/5..10?revert&foo=bar');
params.messageHash = '#123';
assert.equal(element._generateUrl(params), '/c/test/+/1234/5..10#123');
});
test('change with repo name encoding', () => {