Summary

Port the related user selector to ES6

Review Request #8984 — Created June 2, 2017 and submitted June 13, 2017, 3:24 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Blocks

8985

Reviewers

Groups

reviewboard

People

Description

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

Testing Done

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

Issues

Description	From	Last Updated
Col: 23 Missing semicolon.	reviewbot	June 2, 2017, 3:57 p.m.
Col: 22 Expected ')' and instead saw ';'.	reviewbot	June 2, 2017, 3:57 p.m.
Col: 21 Expected an identifier and instead saw ')'.	reviewbot	June 2, 2017, 3:57 p.m.
Col: 21 Unexpected ')'.	reviewbot	June 2, 2017, 3:57 p.m.
Col: 22 Expected ')' and instead saw ';'.	reviewbot	June 2, 2017, 4:06 p.m.
Col: 23 Missing semicolon.	reviewbot	June 2, 2017, 4:06 p.m.
Col: 21 Unexpected ')'.	reviewbot	June 2, 2017, 4:06 p.m.
Col: 21 Expected an identifier and instead saw ')'.	reviewbot	June 2, 2017, 4:06 p.m.
I think with RB 3.0+, we can assume localeCompare exists.	david	June 5, 2017, 4:28 p.m.
Why did you pull this out?	david	June 5, 2017, 4:28 p.m.
Trailing whitespace.	david	June 5, 2017, 4:29 p.m.
Add a trailing comma?	david	June 5, 2017, 4:29 p.m.
Add a trailing comma?	david	June 5, 2017, 4:31 p.m.
this.options.initialOptions.forEach(item => this._onItemSelected(item, false));	david	June 5, 2017, 4:30 p.m.
No spaces around the =	david	June 5, 2017, 4:31 p.m.
Add a trailing comma?	david	June 5, 2017, 4:31 p.m.
Add a trailing comma?	david	June 5, 2017, 4:31 p.m.
Add a trailing comma?	david	June 5, 2017, 4:31 p.m.
Add a trailing comma?	david	June 5, 2017, 4:31 p.m.
Add a trailing comma?	david	June 5, 2017, 4:31 p.m.
Add a trailing comma?	david	June 5, 2017, 4:31 p.m.
Col: 3 Missing semicolon.	reviewbot	June 5, 2017, 4:54 p.m.
While it was like this before, the placeholder text needs to come from the subclass, since the generic implementation shouldn't …	david	June 6, 2017, 1:52 p.m.
Why did you create a default value for this? Is it valid to construct this without passing in options? At …	david	June 6, 2017, 1:52 p.m.
No spaces around =. Same question here about whether options should really be optional.	david	June 6, 2017, 1:52 p.m.
Would be a little bit better as: .click(() => this._onItemRemoved($li, item));	david	June 13, 2017, 3:21 p.m.

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 23
 Missing semicolon.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 22
 Expected ')' and instead saw ';'.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 21
 Expected an identifier and instead saw ')'.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 21
 Unexpected ')'.
```

Description:

~		This change ports the related user & object selctor JS to use ES6. This
	~	This change ports the related user & object selctor JS to use ES6.
-		is part of a multi-patch refactor to allow the user selector to support
-		both single-valued and multi-valued inputs.

		While porting the widget, I refactored
		`RB.RelatedObjectSelectorView.compareStrings` function, which previously
		would check for the presence of `String.prototype.localeCompare` on each
		call, into a thin wrapper around either `localeCompare` (if defined) or
		the fallback method. This way we only have to check for
		`localeCompare`'s presence once.

Diff:

Revision 2 (+103 -95)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
	reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 22
 Expected ')' and instead saw ';'.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 23
 Missing semicolon.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 21
 Unexpected ')'.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 21
 Expected an identifier and instead saw ')'.
```

Diff:

Revision 3 (+103 -95)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
	reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Diff:

Revision 4 (+104 -96)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
	reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Diff:

Revision 5 (+103 -97)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
	reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
I think with RB 3.0+, we can assume localeCompare exists.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 5)
The issue has been dropped. Show all issues
```
Why did you pull this out?
```
1. brennie June 5, 2017, 4:32 p.m.
  We don't need it in the class.
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add a trailing comma?
```
reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add a trailing comma?
```

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 5)

The issue has been resolved. Show all issues


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

reviewboard/static/rb/js/admin/views/relatedUserSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
No spaces around the =
```
reviewboard/static/rb/js/admin/views/relatedUserSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add a trailing comma?
```
reviewboard/static/rb/js/admin/views/relatedUserSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add a trailing comma?
```
reviewboard/static/rb/js/admin/views/relatedUserSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add a trailing comma?
```
reviewboard/static/rb/js/admin/views/relatedUserSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add a trailing comma?
```
reviewboard/static/rb/js/admin/views/relatedUserSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add a trailing comma?
```
reviewboard/static/rb/js/admin/views/relatedUserSelectorView.es6.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add a trailing comma?
```

Change Summary:

Addressed David's feedback.

Diff:

Revision 6 (+88 -108)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
	reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 3
 Missing semicolon.
```

Change Summary:

jshint

Diff:

Revision 7 (+88 -108)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
	reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 7)

The issue has been resolved. 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"

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 7)

The issue has been resolved. 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).

reviewboard/static/rb/js/admin/views/relatedUserSelectorView.es6.js (Diff revision 7)
The issue has been resolved. Show all issues
```
No spaces around =. Same question here about whether options should really be optional.
```
1. brennie June 6, 2017, 1:48 p.m.
  Sorry, just a habit. It isn't optional.

Change Summary:

Addressed David's issues

Diff:

Revision 8 (+92 -105)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.js
	reviewboard/static/rb/js/admin/views/relatedUserSelectorView.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

~		This change ports the related user & object selctor JS to use ES6.
	~	This change ports the related user & object selctor JS to use ES6.
-
-		While porting the widget, I refactored
-		`RB.RelatedObjectSelectorView.compareStrings` function, which previously
-		would check for the presence of `String.prototype.localeCompare` on each
-		call, into a thin wrapper around either `localeCompare` (if defined) or
-		the fallback method. This way we only have to check for
-		`localeCompare`'s presence once.

Ship it!

reviewboard/static/rb/js/admin/views/relatedObjectSelectorView.es6.js (Diff revision 8)

The issue has been resolved. Show all issues

Would be a little bit better as:

.click(() => this._onItemRemoved($li, item));

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (85f4e52)