Summary

Convert the view and model for the diff viewer page to ES6.

Review Request #9152 — Created Aug. 25, 2017 and submitted Sept. 4, 2017, 4:19 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

0acc7cb...

Reviewers

Groups

reviewboard

People

Description

This updates the diff viewer page's view and model to ES6 and fixes up
the docs for our modern standards. The logic for these classes remains
the same, but the code has been streamlined by the usage of template
literals, consts/lets, and fat arrow functions.

Upcoming work will build upon this to simplify the view further.

Testing Done

Unit tests pass.

Tested all functionality of the diff viewer (revision selection,
interdiffs, pagination, anchor navigation, commenting) to ensure
nothing broke.

Issues

Description	From	Last Updated
Args/Returns?	david	Aug. 30, 2017, 4:34 p.m.
Trailing comma?	david	Aug. 30, 2017, 4:35 p.m.
Args?	david	Aug. 30, 2017, 4:35 p.m.
Instead of a function-scoped variable created at the top, how about just setting const parts here?	david	Aug. 30, 2017, 4:35 p.m.
"Return".	david	Aug. 30, 2017, 4:35 p.m.
Convert this to a .forEach loop?	david	Aug. 30, 2017, 4:36 p.m.
forEach?	david	Aug. 30, 2017, 4:36 p.m.
This can be const [base, tip] = revisions;	david	Aug. 30, 2017, 4:37 p.m.
forEach? This can also be a lot more terse: this.diffReviewableViews.forEach(view => view.remove());	david	Aug. 30, 2017, 4:38 p.m.
Trailing comma.	david	Aug. 30, 2017, 4:38 p.m.

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/pages/models/diffViewerPageModel.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Args/Returns?
```
reviewboard/static/rb/js/pages/models/diffViewerPageModel.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing comma?
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Args?
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Instead of a function-scoped variable created at the top, how about just setting const parts here?
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
"Return".
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Convert this to a .forEach loop?
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
forEach?
```

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1)

The issue has been dropped. Show all issues

This can be const [base, tip] = revisions;

chipx86 Aug. 30, 2017, 4:40 p.m.

Going to keep this as-is. const [base, tip] = revisions results in a lot more code in each file that uses it (Babel injects a decent-sized file-level function that checks for different sets of conditions, calls that to normalize the array, and then does exactly what this original code does, but with an additional variable in the scope). This isn't done in any common place, unfortunately, so it ends up ballooning the size of the bundles.
I love that the syntax exists in ES6, but Babel's output isn't at all worth it right now.

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

forEach?
This can also be a lot more terse:

this.diffReviewableViews.forEach(view => view.remove());

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing comma.
```

Change Summary:


Fixed some doc issues.
Added missing trailing commas.
Converted some _.each() statements to .forEach().
Changed the scope of a variable declaration.

Commit:

73fe38b021e6bffd340eed22f95fb01d664128c5

0acc7cbf7b6cbac70939d267fa4d4c714fb51b5d

Diff:

Revision 2 (+356 -249)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.0.x (0e29a8d)