• 
      

    Port the related user selector to ES6

    Review Request #8984 — Created June 2, 2017 and submitted

    Information

    Review Board
    release-3.0.x

    Reviewers

    This change ports the related user & object selctor JS to use ES6.

    The related user selection widget on the repositories change form
    still works as intended.

    Description From Last Updated

    Col: 23 Missing semicolon.

    reviewbotreviewbot

    Col: 22 Expected ')' and instead saw ';'.

    reviewbotreviewbot

    Col: 21 Expected an identifier and instead saw ')'.

    reviewbotreviewbot

    Col: 21 Unexpected ')'.

    reviewbotreviewbot

    Col: 22 Expected ')' and instead saw ';'.

    reviewbotreviewbot

    Col: 23 Missing semicolon.

    reviewbotreviewbot

    Col: 21 Unexpected ')'.

    reviewbotreviewbot

    Col: 21 Expected an identifier and instead saw ')'.

    reviewbotreviewbot

    I think with RB 3.0+, we can assume localeCompare exists.

    daviddavid

    Why did you pull this out?

    daviddavid

    Trailing whitespace.

    daviddavid

    Add a trailing comma?

    daviddavid

    Add a trailing comma?

    daviddavid

    this.options.initialOptions.forEach(item => this._onItemSelected(item, false));

    daviddavid

    No spaces around the =

    daviddavid

    Add a trailing comma?

    daviddavid

    Add a trailing comma?

    daviddavid

    Add a trailing comma?

    daviddavid

    Add a trailing comma?

    daviddavid

    Add a trailing comma?

    daviddavid

    Add a trailing comma?

    daviddavid

    Col: 3 Missing semicolon.

    reviewbotreviewbot

    While it was like this before, the placeholder text needs to come from the subclass, since the generic implementation shouldn't …

    daviddavid

    Why did you create a default value for this? Is it valid to construct this without passing in options? At …

    daviddavid

    No spaces around =. Same question here about whether options should really be optional.

    daviddavid

    Would be a little bit better as: .click(() => this._onItemRemoved($li, item));

    daviddavid
    Checks run (1 failed, 1 succeeded)
    flake8 passed.
    JSHint failed.

    JSHint

    brennie
    brennie
    Review request changed

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    brennie
    brennie
    brennie
    david
    1. 
        
    2. Show all issues

      I think with RB 3.0+, we can assume localeCompare exists.

    3. Show all issues

      Why did you pull this out?

      1. We don't need it in the class.

    4. Show all issues

      Trailing whitespace.

    5. Show all issues

      Add a trailing comma?

    6. Show all issues

      Add a trailing comma?

    7. Show all issues

      this.options.initialOptions.forEach(item =>
          this._onItemSelected(item, false));
      
    8. Show all issues

      No spaces around the =

    9. Show all issues

      Add a trailing comma?

    10. Show all issues

      Add a trailing comma?

    11. Show all issues

      Add a trailing comma?

    12. Show all issues

      Add a trailing comma?

    13. Show all issues

      Add a trailing comma?

    14. Show all issues

      Add a trailing comma?

    15. 
        
    brennie
    Review request changed
    Change Summary:

    Addressed David's feedback.

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    brennie
    david
    1. 
        
    2. Show all issues

      While it was like this before, the placeholder text needs to come from the subclass, since the generic implementation shouldn't have "Search for users"

    3. Show all issues

      Why did you create a default value for this? Is it valid to construct this without passing in options?

      At the least, it looks like we'd need to have a default value for selectizeOptions (since _.defaults(undefined, { ... }) will return undefined).

    4. Show all issues

      No spaces around =. Same question here about whether options should really be optional.

      1. Sorry, just a habit. It isn't optional.

    5. 
        
    brennie
    brennie
    david
    1. 
        
    2. Show all issues

      Would be a little bit better as:

      .click(() => this._onItemRemoved($li, item));
      
    3. 
        
    brennie
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (85f4e52)