Fish Trophy

david got a 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)
     
     
     
     
     
     
     
     

    These are missing docs.

  3. 
      
david
maubin
  1. 
      
  2. 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. Do we want any of this to be in onInitialRender?

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

    Can we document these?

  4. We're using this style elsewhere:

    export class CenteredElementManager extends BaseView<
        ...,
        ...
    > {
        ...
    }
    
  5. 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<...>

  6. Should we move this into onInitialRender?

  7. Can we list the version this will be removed?

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

  9. Can you add docs for this?

  10. 
      
david
chipx86
  1. 
      
  2. The new docs need a Version Added (for anything new, of course).

  3. 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: Closed (submitted)

Change Summary:

Pushed to release-7.x (5ebe971)
Loading...