Summary

Clean up some option plumbing through the review box views.

Review Request #8317 — Created Aug. 3, 2016 and submitted Aug. 8, 2016, 2:25 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

3327fa2...

Reviewers

Groups

reviewboard

People

Description

Some of the options passed through the various levels of views making up the
review list were named a little oddly, or could be made more specific.

Testing Done

Ran js-tests.

Issues

Description	From	Last Updated
No trailing comma for non-ES6 files.	chipx86	Aug. 5, 2016, 11:54 a.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxListView.es6.js
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.es6.js
    reviewboard/static/rb/js/views/tests/reviewBoxViewTests.js
    reviewboard/static/rb/js/views/tests/reviewBoxListViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/js/views/reviewBoxView.es6.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewBoxListView.es6.js
    reviewboard/static/rb/js/pages/views/reviewRequestPageView.es6.js
    reviewboard/static/rb/js/views/tests/reviewBoxViewTests.js
    reviewboard/static/rb/js/views/tests/reviewBoxListViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/js/views/reviewBoxView.es6.js

Ship it!

```
Ship It!
```

Something to verify is that we're not breaking anybody with this change. Mozilla's more likely than most to be affected by this, so we could check with them.

If needed, we could have compatibility attributes for these. I think the big one would be pageEditState, which we may really want to alias for now and document as deprecated. (Maybe we want to put together a page in Notion just to list these, so we can better assemble into release notes?)

david Aug. 5, 2016, noon

I'd love to have a clear picture of what constitutes our "API" and what constitues our "implementation" :(

Added notes to https://www.notion.so/Review-Request-page-changes-for-3-0-8d27aac9e3ed4f81ac1e849ebb6127fe

chipx86 Aug. 8, 2016, 1:01 p.m.

Yeah, we really don't have that very well defined... I'm just going off of code and discussions I've seen.

The page, its state, and its objects should probably constitute API, though. That stuff would be common across pages with review UIs or JS-side extensions that need to grab information on the review request or edit state.

reviewboard/static/rb/js/views/tests/reviewBoxViewTests.js (Diff revision 1)

The issue has been dropped. Show all issues

No trailing comma for non-ES6 files.

david Aug. 5, 2016, 10:35 a.m.

I don't see any good argument for restricting trailing commas to ES6. The transpiler doesn't do anything with them, and they were only a problem in IE8, which we're no longer supporting.

david Aug. 5, 2016, 3:23 p.m.

OK, I suppose I'm willing to only do this for ES6 files if only to avoid an argument, but this file itself is deleted in a dependent change, so I'm not going to fix this instance.

chipx86 Aug. 8, 2016, 1:01 p.m.

Sounds good.

I'd be happy to move to trailing commas everywhere, but when researching this, it looked like IE9 could still have unexpected behavior/crashes at times, depending on the mode it's in. Newer versions work better. So at some point, when IE9 is a distant memory (not yet, sadly), we should be in good shape to use trailing commas everywhere.

Status:: Completed
Change Summary:: Pushed to release-3.0.x (087fece)