Convert comment block views to TypeScript/spina.

Review Request #13527 — Created Feb. 6, 2024 and submitted

Information

Review Board
master

Reviewers

This change converts the comment block view classes to TypeScript.

  • Ran js-tests.
  • Smoke tested commenting on diffs and various Review UIs.
Summary ID
Convert comment block views to TypeScript/spina.
This change converts the comment block view classes to TypeScript. Testing Done: - Ran js-tests. - Smoke tested commenting on diffs and various Review UIs. Reviewed at https://reviews.reviewboard.org/r/13527/
b24b677ea58c0b35a7a0c9acd67924a6f203414f
Description From Last Updated

I notice there's a lot of places in the old code where we could pull out things like this.model and …

maubinmaubin

To keep this formatted similarly to other classes and deal with the indent, you could do: export class DiffCommentBlockView extends …

chipx86chipx86

This could be moved into static modelEvents = { ... }. If not that, then this should be changed to …

chipx86chipx86

These are missing docs.

chipx86chipx86

Missing Version Added.

chipx86chipx86

These are missing docs.

chipx86chipx86

These should be moved into modelEvents.

chipx86chipx86

Let's pull #moveState into a local variable. No sense in looking it up 8 times.

chipx86chipx86

Could use an f-string here.

chipx86chipx86

Could use f-strings here.

chipx86chipx86
maubin
  1. 
      
  2. Show all issues

    I notice there's a lot of places in the old code where we could pull out things like this.model and this.$el into variables to avoid repeated lookups. Not really critical to change this, but you could tackle it while you're here if you want.

    1. I think this change is already big enough but I'll make a note of it.

  3. 
      
chipx86
  1. 
      
  2. Show all issues

    To keep this formatted similarly to other classes and deal with the indent, you could do:

    export class DiffCommentBlockView extends TextBasedCommentBlockView<
        DiffCommentBlock
    > {
        ...
    }
    
  3. Show all issues

    This could be moved into static modelEvents = { ... }.

    If not that, then this should be changed to use .listenTo() instead of .on().

  4. reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    These are missing docs.

  5. Show all issues

    Missing Version Added.

    1. This is purely internal API within the RegionCommentBlockView. I don't think we care about documenting versions for that?

    2. I think it's useful for code archaeology when trying to figure out when stuff changed. I try to document anything new, regardless if it's for internal or public consumption, so it's always clear.

  6. reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These are missing docs.

    1. So I'm fine adding explanations for the types as a whole, but these interfaces and types are purely internal to RegionCommentBlockView, and really just exist so that we get type checking and consistency within the implementation. I don't think we gain anything by adding a bunch of comments that mostly just reflect the same name as the attributes.

  7. Show all issues

    These should be moved into modelEvents.

  8. reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts (Diff revision 1)
     
     
     
     
     
     
     
     
     
    Show all issues

    Let's pull #moveState into a local variable. No sense in looking it up 8 times.

  9. Show all issues

    Could use an f-string here.

    1. This is TS, not Python, but I know what you meant.

    2. We're gonna have to find a Python-to-TS converter so I can make my comment make more sense.

  10. Show all issues

    Could use f-strings here.

  11. 
      
david
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.x (accb55b)