Port the related user selector to ES6

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

brennie
Review Board
release-3.0.x
8985
reviewboard

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. I think with RB 3.0+, we can assume localeCompare exists.

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

  3. this.options.initialOptions.forEach(item =>
        this._onItemSelected(item, false));
    
  4. 
      
brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

brennie
david
  1. 
      
  2. 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. 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. 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. Would be a little bit better as:

    .click(() => this._onItemRemoved($li, item));
    
  3. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (85f4e52)
Loading...