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)