Add event bubbling support for Infoboxes

Review Request #9306 - Created Oct. 22, 2017 and updated

Shawn Lamothe
Review Board
master
350229a...
reviewboard, students

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.

  • 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.
  • 1
  • 0
  • 12
  • 0
  • 13
Description From Last Updated
Does it have to be a jQuery element? I see on line 108 you're passing it to $. Doesn't that ... Mike Conley Mike Conley
Checks run (1 failed, 1 succeeded)
flake8 passed.
JSHint failed.

JSHint

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

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

  2. Typo in your description:

    "inneficient" -> "inefficient".

  3. Another typo in your description:

    addContaineSelector -> addContainerSelector.

  4. One last description typo:

    "specefied" -> "specified"

  5. 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.

    1. 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."

    2. 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.

  6. 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?

    1. 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?

    2. 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.

    3. 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?

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

  7. Nit: "showed" -> "shown"

  8. 
      
Shawn Lamothe
Shawn Lamothe
Review request changed

Commit:

-aed2e2dce131d32d3a4dd9dd5bcbddb5539fdc73
+350229aac9567673dab940a3ef25dbeb35f284eb

Diff:

Revision 2 (+132)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Shawn Lamothe
  1. 
      
  2. 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.

  3. 
      
Loading...