Summary

Add event bubbling support for Infoboxes

Review Request #9306 — Created Oct. 22, 2017 and discarded Jan. 29, 2019, 1:46 p.m.

Information

Owner

slamo

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

Currently the only mechanism to register Infoboxes is to pass in a list of elements to InfoboxManagerView.addTargets. This creates event listeners on each individual element. This mechanism is okay when the number of elements is small, however becomes ineficient when needing to listen for events on a large number of elements.

This change adds a new mechanism addContainerSelector to InfoboxManagerView. Instead of adding listeners to each element, addContainerSelector registers a single listener on the parent container which listens and triggers events for the children specified in the selector.

Testing Done


Existing unit tests passed
Two new unit tests passed
Manual testing: created a dummy infobox type and added it to DiffReviewableView on all '.n' Pygments classes. Ensured events triggered when hovering over specified classes, but not when hovering over non-specified classes. This manual testing only covered the mouseenter events and not the mouseleave events.

Issues

Description	From	Last Updated
Typo in your description: "inneficient" -> "inefficient".	mike_conley	Oct. 22, 2017, 1:42 p.m.
Another typo in your description: addContaineSelector -> addContainerSelector.	mike_conley	Oct. 22, 2017, 1:42 p.m.
One last description typo: "specefied" -> "specified"	mike_conley	Oct. 22, 2017, 1:43 p.m.
Nit: that's a lot of the use of the word "specified". How about: "Hovering over one of the children in …	mike_conley	Oct. 27, 2017, 8:01 a.m.
Does it have to be a jQuery element? I see on line 108 you're passing it to $. Doesn't that …	mike_conley	Oct. 22, 2017, 1:10 p.m.
Nit: "showed" -> "shown"	mike_conley	Oct. 22, 2017, 1:40 p.m.
Nit: space after //	mike_conley	Oct. 22, 2017, 1:40 p.m.
Col: 36 ['args'] is better written in dot notation.	reviewbot	Oct. 22, 2017, 1:28 p.m.
Nit: space after //	mike_conley	Oct. 22, 2017, 1:40 p.m.
Nit: space after //	mike_conley	Oct. 22, 2017, 1:40 p.m.
Col: 36 ['args'] is better written in dot notation.	reviewbot	Oct. 22, 2017, 1:28 p.m.
Nit: space after //	mike_conley	Oct. 22, 2017, 1:40 p.m.
Col: 36 ['args'] is better written in dot notation.	reviewbot	Oct. 22, 2017, 1:28 p.m.

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/ui/views/tests/infoboxManagerViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 36
 ['args'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/infoboxManagerViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 36
 ['args'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/infoboxManagerViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 36
 ['args'] is better written in dot notation.
```

Fix it!

This looks really good, @slamo - just a few minor things I found; all cosmetic.

slamo Oct. 22, 2017, 4:52 p.m.

Is there spellcheck support for the descriptions/testing done fields? My typing is helplessly filled with typos.

The issue has been resolved. Show all issues

Typo in your description:

"inneficient" -> "inefficient".

The issue has been resolved. Show all issues

Another typo in your description:

addContaineSelector -> addContainerSelector.

The issue has been resolved. Show all issues

One last description typo:

"specefied" -> "specified"

reviewboard/static/rb/js/ui/views/infoboxManagerView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Nit: that's a lot of the use of the word "specified". How about:

"Hovering over one of the children in the container will trigger the specified infobox." ?

I think that conveys the same meaning.

slamo Oct. 22, 2017, 1:45 p.m.

Well written doc string comments are an area I need some work in, what about:

"Add a container that watches a set of children for a particular type of infobox.

Hovering over one of the specified children in the container will trigger the infobox."

mike_conley Oct. 23, 2017, 7:25 a.m.

My only complaint is the "add a container" part - that seems to imply that a container node is going to be created and appended somewhere into the DOM.

reviewboard/static/rb/js/ui/views/infoboxManagerView.es6.js (Diff revision 1)

An issue was opened. Show all issues

Does it have to be a jQuery element? I see on line 108 you're passing it to $. Doesn't that mean it could be a raw DOM node?

slamo Oct. 22, 2017, 1:45 p.m.

This is me being unfamiliar with jQuery. Would it be better to force it to be a jQuery element and take ou the $ on 108, or allow it to be a raw DOM node and change the documentation accordingly?

mike_conley Oct. 23, 2017, 7:25 a.m.

Hm. My personal preference is to be liberal in what a function accepts, but conservative in what it returns. So I'd be find with it accepting either the DOM node or the jQuery element. Best to sniff around the codebase though to see if there are previous examples of this happening.

slamo Oct. 27, 2017, 8:40 a.m.

I looked into what the current method addTargets is doing. The documentation specifies jQuery element, and in all cases they pass in a set of jQuery elements but the function starts by calling
$targets.each((idx, target) => {
            const $target = $(target);


If you call $() on a jQuery element does it return itself, or a new jQuery element representing the same thing?

mike_conley Nov. 12, 2017, 2:10 p.m.

I'm 90% certain that if you pass a jQuery object to $, you get the same object back.

reviewboard/static/rb/js/ui/views/infoboxManagerView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Nit: "showed" -> "shown"
```
reviewboard/static/rb/js/ui/views/tests/infoboxManagerViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Nit: space after //
```
reviewboard/static/rb/js/ui/views/tests/infoboxManagerViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Nit: space after //
```
reviewboard/static/rb/js/ui/views/tests/infoboxManagerViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Nit: space after //
```
reviewboard/static/rb/js/ui/views/tests/infoboxManagerViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Nit: space after //
```

Description:: ~
Currently the only mechanism to register Infoboxes is to pass in a list of elements to InfoboxManagerView.addTargets. This creates event listeners on each individual element. This mechanism is okay when the number of elements is small, however becomes inneficient when needing to listen for events on a large number of elements.
~
Currently the only mechanism to register Infoboxes is to pass in a list of elements to InfoboxManagerView.addTargets. This creates event listeners on each individual element. This mechanism is okay when the number of elements is small, however becomes ineficient when needing to listen for events on a large number of elements.

~
This change adds a new mechanism addContainerSelector to InfoboxManagerView. Instead of adding listeners to each element, addContaineSelector registers a single listener on the parent container which listens and triggers events for the children specefied in the selector.
~
This change adds a new mechanism addContainerSelector to InfoboxManagerView. Instead of adding listeners to each element, addContainerSelector registers a single listener on the parent container which listens and triggers events for the children specified in the selector.

Commit:

aed2e2dce131d32d3a4dd9dd5bcbddb5539fdc73

350229aac9567673dab940a3ef25dbeb35f284eb

Diff:

Revision 2 (+132)

Show changes

	reviewboard/static/rb/js/ui/views/infoboxManagerView.es6.js
	reviewboard/static/rb/js/ui/views/tests/infoboxManagerViewTests.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/ui/views/tests/infoboxManagerViewTests.es6.js (Diff revision 1)

I was originally trying to do this using expect(infoboxManagerView._onTargetMouseEnter).toHaveBeenCalledWith($fooElement, DummyInfoboxView) however the $target in the code had a context, whereas $fooElement did not. This was my work-around for ensuring that the span being passed was the correct one. If there is a nicer way, or a way for me to have the span with a context in the test code, please let me know.

Status:: Discarded