Summary

Update RB.DiffReviewableView to modern standards

Review Request #10194 — Created Oct. 4, 2018 and submitted Oct. 18, 2018, 9:39 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Commit

0d3a1df...

Reviewers

Groups

reviewboard

People

Description

This patch updates RB.DiffReviewableView and its unit tests to use

ES6 features and our documentation standards. Although the

RB.DiffReviewableView was written in an ES6 file, it was still using

var everywhere over let and const. It was also wrapped in a

superfluous IIFE.

Testing Done

Ran JS tests.

Issues

Description	From	Last Updated
Typo in description: Althought	david	Oct. 15, 2018, 1:20 p.m.
Indentation is funky here.	david	Oct. 15, 2018, 1:20 p.m.
This could be: $children.first().toggleClass('first'); $children.last().toggleClass('last');	david	Oct. 15, 2018, 1:20 p.m.
Extra blank line.	chipx86	Oct. 16, 2018, 2:30 p.m.
Can we keep one param per line here? Just nicer to read it that way.	chipx86	Oct. 16, 2018, 2:30 p.m.
Missing docs.	chipx86	Oct. 16, 2018, 2:33 p.m.
Missing Args.	chipx86	Oct. 16, 2018, 2:33 p.m.
"Handle"	chipx86	Oct. 16, 2018, 2:33 p.m.
This does not English good.	chipx86	Oct. 16, 2018, 2:33 p.m.
Missing a blank line here.	chipx86	Oct. 18, 2018, 9:10 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues
```
Typo in description: Althought
```

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Indentation is funky here.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

This could be:

$children.first().toggleClass('first');
$children.last().toggleClass('last');

Change Summary:

Addressed feedback.

Description:

		This patch updates `RB.DiffReviewableView` and its unit tests to use
~		ES6 features and our documentation standards. Althought the
	~	ES6 features and our documentation standards. Although the
		`RB.DiffReviewableView` was written in an ES6 file, it was still using
		`var` everywhere over `let` and `const`. It was also wrapped in a
		superfluous IIFE.

Commit:

6e6847a8480e74f85d2fce3903fa88312b0b41ff

e715ff66375607e37350594615b0c18177f7b258

Diff:

Revision 2 (+322 -288)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

Extra blank line.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we keep one param per line here? Just nicer to read it that way.
```
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing docs.
```

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

Missing Args.

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
"Handle"
```
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This does not English good.
```

Change Summary:

Addressed Feedback.

Commit:

e715ff66375607e37350594615b0c18177f7b258

0d3a1df64d23709ab04a7460a53bf1a018646959

Diff:

Revision 3 (+329 -288)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js
	reviewboard/static/rb/js/diffviewer/views/tests/diffReviewableViewTests.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/static/rb/js/diffviewer/views/diffReviewableView.es6.js (Diff revision 3)

The issue has been resolved. Show all issues

Missing a blank line here.

Status:: Completed
Change Summary:: Pushed to release-4.0.x (af514f1)