• 
      

    Add a searchPrefix option to rbautocomplete.

    Review Request #6807 — Created Jan. 18, 2015 and discarded

    Information

    Review Board
    master

    Reviewers

    There are some installs of Review Board where the user list is so large,
    with so many similar names, that it's necessary to find a new way of
    differentiating them when searching for their entries for autocomplete.
    
    An example of this is Mozilla's instance of Review Board. Mozilla's
    convention is to have the fullnames of their users followed by their
    IRC Nick, prefixed with a colon, for example:
    
    Mike Conley (:mconley)
    
    RB's autocomplete widget isn't prepared to handle searches for ":mconley",
    and map them to the actual Review Board username "mconley". This works
    around that issue.
    
    Really, this just supports the perpetuation of a habit that Mozillians
    have developed over time. It's true that they could just start entering
    the IRC nick and get the result they're looking for, but the colon prefix
    is what they're used to.

    In a local instance, I made sure that searching for normal usernames (without a search prefix) worked properly to make sure we didn't regress. I tested both selecting an autocomplete entry by pressing "enter" and "tab", as well as selecting an entry with the mouse.

    Then, in our RBMozUI extension, where we've forked rbautocomplete with this patch, I used searchPrefix to make sure the colon prefix work around still worked. I was able to autocomplete with the colon prefix, and was able to select a reviewer both by pressing "enter" and "tab", as well as selecting one with the mouse.

    Description From Last Updated

    In line with our style guide, shouldn't there be only one var declaration and it should occur at the top …

    brenniebrennie

    Shouldn't have a trailing comma here.

    daviddavid

    The var definition for trimmed should actually be inside here (since there's an anonymous function). I think the comment was …

    daviddavid

    Should use ===

    daviddavid
    reviewbot
    1. Tool: Pyflakes
      Ignored Files:
          reviewboard/static/lib/js/ui.autocomplete.js
      
      
      
      Tool: PEP8 Style Checker
      Ignored Files:
          reviewboard/static/lib/js/ui.autocomplete.js
      
      
    2. 
        
    brennie
    1. 
        
    2. Show all issues

      In line with our style guide, shouldn't there be only one var declaration and it should occur at the top of the function?

      Given that this is a fork of some jQuery library, I'm not sure if this applies or not (and/or whether it applies to the var statements above).

      1. Thanks for the review! Which part of the style guide are you referring to? I checked here and didn't see anything in there about JavaScript, or location of variable declarations.

        I'm also not sure what we want to do with this fork of ui.autocomplete. It's kind of a jungle in here.

      2. Apparently we don't have a js style guide :). I was going by the existing code and changes (à la https://reviews.reviewboard.org/r/6768/diff/ diffFragmentQueueView.js).

      3. We generally try to make jshint happy. For this file I wouldn't worry too much because it started out as such a big mess.

      4. Alright then, feel free to drop this issue.

      5. Or rather, I'll drop it. Forgot I can :p

      6. It should though, yes. We declare vars at the top of the function (due to variable hoisting in JS) and we use only one var to help minifiers and just for consistency.

      7. Ah, alright. I'll update the review request then. I forgot about var hoisting. I can't wait until ES6 let is used everywhere.

    3. 
        
    brennie
    1. Ship It!
    2. 
        
    mike_conley
    reviewbot
    1. Tool: PEP8 Style Checker
      Ignored Files:
          reviewboard/static/lib/js/ui.autocomplete.js
      
      
      
      Tool: Pyflakes
      Ignored Files:
          reviewboard/static/lib/js/ui.autocomplete.js
      
      
    2. 
        
    david
    1. 
        
    2. Show all issues

      Shouldn't have a trailing comma here.

    3. Show all issues

      The var definition for trimmed should actually be inside here (since there's an anonymous function). I think the comment was really that words and result should be defined in a single var statement at the top of the trimWords function.

    4. Show all issues

      Should use ===

    5. 
        
    david
    Review request changed
    Status:
    Discarded