Fish Trophy

david got a fish trophy!

Fish Trophy

Convert reviewable views to TypeScript/spina.

Review Request #13531 — Created Feb. 12, 2024 and submitted

Information

Review Board
master

Reviewers

This change converts the reviewable views (and associated utility views)
to TypeScript/spina.

While I was here I disconvered that one of the tests for DiffReviewable
was intended to be async, but didn't take a done callback and was
therefore skipping the meat of the test. After fixing that, it became
evident that the implementation of the test was broken, so it's been
rewritten.

  • Ran js-tests.
  • Smoke tested reviewing diffs and file attachments.
Summary ID
Convert reviewable views to TypeScript/spina.
This change converts the reviewable views (and associated utility views) to TypeScript/spina. While I was here I disconvered that one of the tests for DiffReviewable was intended to be async, but didn't take a `done` callback and was therefore skipping the meat of the test. After fixing that, it became evident that the implementation of the test was broken, so it's been rewritten. Testing Done: - Ran js-tests. - Smoke tested reviewing diffs and file attachments. Reviewed at https://reviews.reviewboard.org/r/13531/
b532967c60148688cb442555fba4439cebbcc1c3
Description From Last Updated

Nit: I notice there's still some places where we use RB.DiffReviewableView in docstrings (mostly for argument types and return types), …

maubinmaubin

These are missing docs.

maubinmaubin

Do we want any of this to be in onInitialRender?

chipx86chipx86

Can we document these?

chipx86chipx86

Can we document this?

chipx86chipx86

We're using this style elsewhere: export class CenteredElementManager extends BaseView< ..., ... > { ... }

chipx86chipx86

Should be Partial<...>.

chipx86chipx86

Should we move this into onInitialRender?

chipx86chipx86

Can we list the version this will be removed?

chipx86chipx86

Missing docs.

chipx86chipx86

The new docs need a Version Added (for anything new, of course).

chipx86chipx86

Can we document these, since they're meant to be filled in by subclasses?

chipx86chipx86

Can you add docs for this?

chipx86chipx86

Hmm, I should have exposed this in Spina, if we need this. Do we, though? We're only going to use …

chipx86chipx86

Should be Partial<...>.

chipx86chipx86
maubin
  1. 
      
  2. reviewboard/static/rb/js/reviews/views/textBasedReviewableView.ts (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    These are missing docs.

  3. 
      
david
maubin
  1. 
      
  2. Show all issues

    Nit: I notice there's still some places where we use RB.DiffReviewableView in docstrings (mostly for argument types and return types), should we switch it all over to DiffReviewableView?

    1. I think for doc comments we want to keep it as RB., because that's what it ends up getting exposed as in the final bundle.

  3. 
      
maubin
  1. Ship It!
  2. 
      
chipx86
  1. This looks great. I just spotted a few missing docs, some possible render() -> onInitialRender() cases, and a couple cases where we should use Partial<...> for options.

  2. Show all issues

    Do we want any of this to be in onInitialRender?

  3. reviewboard/static/rb/js/ui/views/centeredElementManager.ts (Diff revision 2)
     
     
     
     
     
     
     
     
    Show all issues

    Can we document these?

  4. Show all issues

    Can we document this?

  5. Show all issues

    We're using this style elsewhere:

    export class CenteredElementManager extends BaseView<
        ...,
        ...
    > {
        ...
    }
    
  6. Show all issues

    Should be Partial<...>.

    1. I actually don't think Partial is correct here. I think the right thing is to have the options interface define any optional fields as optional, and then this should actually be Backbone.CombinedViewConstructorOptions<...>

  7. Show all issues

    Should we move this into onInitialRender?

  8. Show all issues

    Can we list the version this will be removed?

  9. Show all issues

    Missing docs.

  10. Show all issues

    Can we document these, since they're meant to be filled in by subclasses?

  11. Show all issues

    Can you add docs for this?

  12. Show all issues

    Should be Partial<...>.

  13. 
      
david
chipx86
  1. 
      
  2. Show all issues

    The new docs need a Version Added (for anything new, of course).

  3. Show all issues

    Hmm, I should have exposed this in Spina, if we need this.

    Do we, though? We're only going to use what's in our own options. The default Backbone ones are going to be for things like tagName and className, and presumably we don't care about this within initialize (it won't affect construction, which always takes the merged versions).

  4. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.x (5ebe971)