Add event bubbling support for Infoboxes

Review Request #9306 — Created Oct. 22, 2017 and discarded

Information

Review Board
master

Reviewers

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.
Description From Last Updated

Typo in your description: "inneficient" -> "inefficient".

mike_conleymike_conley

Another typo in your description: addContaineSelector -> addContainerSelector.

mike_conleymike_conley

One last description typo: "specefied" -> "specified"

mike_conleymike_conley

Nit: that's a lot of the use of the word "specified". How about: "Hovering over one of the children in …

mike_conleymike_conley

Does it have to be a jQuery element? I see on line 108 you're passing it to $. Doesn't that …

mike_conleymike_conley

Nit: "showed" -> "shown"

mike_conleymike_conley

Nit: space after //

mike_conleymike_conley

Col: 36 ['args'] is better written in dot notation.

reviewbotreviewbot

Nit: space after //

mike_conleymike_conley

Nit: space after //

mike_conleymike_conley

Col: 36 ['args'] is better written in dot notation.

reviewbotreviewbot

Nit: space after //

mike_conleymike_conley

Col: 36 ['args'] is better written in dot notation.

reviewbotreviewbot
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. Show all issues

    Typo in your description:

    "inneficient" -> "inefficient".

  3. Show all issues

    Another typo in your description:

    addContaineSelector -> addContainerSelector.

  4. Show all issues

    One last description typo:

    "specefied" -> "specified"

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

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

    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. Show all issues

    Nit: "showed" -> "shown"

  8. Show all issues

    Nit: space after //

  9. Show all issues

    Nit: space after //

  10. Show all issues

    Nit: space after //

  11. Show all issues

    Nit: space after //

  12. 
      
SL
SL
SL
  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. 
      
david
Review request changed
Status:
Discarded