Move hover handling out of gr-diff-comment

The only code that is interested in hover events from gr-comment-diff is
in gr-diff-highlight, which is used only by gr-diff. For these reasons,
it makes more sense for gr-diff (or gr-diff-highlight) to subscribe to
the native hover events of the comment threads associated with it, and
not require gr-comment to expose any custom events for that. This also
has the benefit that it reduces the requirements on thread elements:
They can just fire regular mouseenter and mouseleave events like any
DOMElement would. This makes it easier to use gr-diff with different
comment thread element implementations.

I used this change to also make the unit tests for this more meaningful
and not stub out all the interesting parts.

Change-Id: I1a969bb6a7092bc433039662cd2034dd7141e4ca
This commit is contained in:
Ole Rehmsen
2018-11-16 14:44:45 +01:00
parent a4641825be
commit e5c377cf7a
5 changed files with 63 additions and 32 deletions

View File

@@ -232,10 +232,7 @@ limitations under the License.
display: block;
}
</style>
<div id="container"
class="container"
on-mouseenter="_handleMouseEnter"
on-mouseleave="_handleMouseLeave">
<div id="container" class="container">
<div class="header" id="header" on-tap="_handleToggleCollapsed">
<div class="headerLeft">
<span class="authorName">[[comment.author.name]]</span>

View File

@@ -58,14 +58,6 @@
* @event comment-update
*/
/**
* @event comment-mouse-over
*/
/**
* @event comment-mouse-out
*/
/**
* Fired when the comment's timestamp is tapped.
*
@@ -610,14 +602,6 @@
}
},
_handleMouseEnter(e) {
this.fire('comment-mouse-over', this._getEventPayload());
},
_handleMouseLeave(e) {
this.fire('comment-mouse-out', this._getEventPayload());
},
_handleToggleResolved() {
this.$.reporting.recordDraftInteraction();
this.resolved = !this.resolved;

View File

@@ -37,8 +37,8 @@
},
listeners: {
'comment-mouse-out': '_handleCommentMouseOut',
'comment-mouse-over': '_handleCommentMouseOver',
'comment-thread-mouseleave': '_handleCommentThreadMouseleave',
'comment-thread-mouseenter': '_handleCommentThreadMouseenter',
'create-range-comment': '_createRangeComment',
},
@@ -74,7 +74,7 @@
this.debounce('selectionChange', this._handleSelection, 200);
},
_handleCommentMouseOver(e) {
_handleCommentThreadMouseenter(e) {
const threadEl = Polymer.dom(e).localTarget;
const index = this._indexForThreadEl(threadEl);
@@ -83,7 +83,7 @@
}
},
_handleCommentMouseOut(e) {
_handleCommentThreadMouseleave(e) {
const threadEl = Polymer.dom(e).localTarget;
const index = this._indexForThreadEl(threadEl);

View File

@@ -197,22 +197,57 @@ limitations under the License.
element._cachedDiffBuilder = builder;
});
test('comment-mouse-over from line comments is ignored', () => {
test('comment-thread-mouseenter from line comments is ignored', () => {
const threadEl = document.createElement('div');
threadEl.setAttribute('comment-side', 'right');
threadEl.setAttribute('line-num', 3);
element.appendChild(threadEl);
element.commentRanges = [{side: 'right'}];
sandbox.stub(element, 'set');
element.fire('comment-mouse-over', {comment: {}});
threadEl.dispatchEvent(
new CustomEvent('comment-thread-mouseenter', {bubbles: true}));
assert.isFalse(element.set.called);
});
test('comment-mouse-over from ranged comment causes set', () => {
test('comment-thread-mouseenter from ranged comment causes set', () => {
const threadEl = document.createElement('div');
threadEl.setAttribute('comment-side', 'right');
threadEl.setAttribute('line-num', 3);
threadEl.setAttribute('range', JSON.stringify({
start_line: 3,
start_character: 4,
end_line: 5,
end_character: 6,
}));
element.appendChild(threadEl);
element.commentRanges = [{side: 'right', range: {
start_line: 3,
start_character: 4,
end_line: 5,
end_character: 6,
}}];
sandbox.stub(element, 'set');
sandbox.stub(element, '_indexForThreadEl').returns(0);
element.fire('comment-mouse-over', {comment: {range: {}}});
threadEl.dispatchEvent(
new CustomEvent('comment-thread-mouseenter', {bubbles: true}));
assert.isTrue(element.set.called);
const args = element.set.lastCall.args;
assert.deepEqual(args[0], ['commentRanges', 0, 'hovering']);
assert.deepEqual(args[1], true);
});
test('comment-mouse-out from line comments is ignored', () => {
element.fire('comment-mouse-over', {comment: {}});
assert.isFalse(builder.getContentsByLineRange.called);
test('comment-thread-mouseleave from line comments is ignored', () => {
const threadEl = document.createElement('div');
threadEl.setAttribute('comment-side', 'right');
threadEl.setAttribute('line-num', 3);
element.appendChild(threadEl);
element.commentRanges = [{side: 'right'}];
sandbox.stub(element, 'set');
threadEl.dispatchEvent(
new CustomEvent('comment-thread-mouseleave', {bubbles: true}));
assert.isFalse(element.set.called);
});
test('on create-range-comment action box is removed', () => {

View File

@@ -250,6 +250,7 @@
// this situation until it occurs.
this._updateRanges(addedThreadEls);
this._updateKeyLocations(addedThreadEls);
this._redispatchHoverEvents(addedThreadEls);
});
},
@@ -274,6 +275,20 @@
}
},
// Dispatch events that are handled by the gr-diff-highlight.
_redispatchHoverEvents(addedThreadEls) {
for (const threadEl of addedThreadEls) {
threadEl.addEventListener('mouseenter', () => {
threadEl.dispatchEvent(
new CustomEvent('comment-thread-mouseenter', {bubbles: true}));
});
threadEl.addEventListener('mouseleave', () => {
threadEl.dispatchEvent(
new CustomEvent('comment-thread-mouseleave', {bubbles: true}));
});
}
},
/** Cancel any remaining diff builder rendering work. */
cancel() {
this.$.diffBuilder.cancel();