Convert ReviewRequestEditorModel to ES6.

Review Request #9147 — Created Aug. 23, 2017 and submitted

Information

Review Board
release-3.0.x
1d50cc7...

Reviewers

Another simple conversion of code that I'm working on.

Ran js-tests.

Description From Last Updated

Missing period.

brenniebrennie

Should we instead be building an array and calling .join('')? Also: ` and "${value}"`

brenniebrennie

`"${value}"`

brenniebrennie

This should probably still pass in the function directly. No sense creating a wrapper function that does nothing but call …

chipx86chipx86

.bind(this) is implied when using this.listenTo().

chipx86chipx86

Same here.

chipx86chipx86

Same here.

chipx86chipx86

I liked using options much better, because it allows room for expansion. I'm not sure getDraftField should assume that useExtraData …

chipx86chipx86

Why the direct comparison to true?

chipx86chipx86

This could use template literals to compose the string.

chipx86chipx86

Same here.

chipx86chipx86

Sentence casing.

chipx86chipx86

This could be one line.

chipx86chipx86
brennie
  1. 
      
  2. Show all issues

    Missing period.

  3. Show all issues

    Should we instead be building an array and calling .join('')?

    Also:

    ` and "${value}"`
    
    1. I don't think we really need to care that much about performance of building the string in this case. This gets called once in the error case.

  4. Show all issues

    `"${value}"`
    
  5. 
      
david
chipx86
  1. 
      
  2. Show all issues

    This should probably still pass in the function directly. No sense creating a wrapper function that does nothing but call the function that listenTo would call itself.

  3. Show all issues

    Same here.

  4. Show all issues

    I liked using options much better, because it allows room for expansion. I'm not sure getDraftField should assume that useExtraData is the only field-related option that's going to come up (or that it should be so prominent). Can we go back to options?

  5. Show all issues

    Why the direct comparison to true?

  6. Show all issues

    This could use template literals to compose the string.

  7. Show all issues

    Same here.

  8. Show all issues

    This could be one line.

  9. 
      
david
chipx86
  1. 
      
  2. Show all issues

    .bind(this) is implied when using this.listenTo().

  3. Show all issues

    Same here.

  4. Show all issues

    Sentence casing.

  5. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.0.x (a5ae0ec)