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

  





 
 

  david
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
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




   

   







  

 



 

  


   

   

    
 
maubin


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/static/rb/js/reviews/views/textBasedReviewableView.ts
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    These are missing docs.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




maubin




   

   







  

 



 

  


   

   

    
 
maubin


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 







  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.
      
 
      

      2. 


 
    


    

 
    

    3. 













  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 





 

  

   




maubin




   

   







  

 



 

  


   

   

    
 
maubin


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
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. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Do we want any of this to be in onInitialRender?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/static/rb/js/ui/views/centeredElementManager.ts
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can we document these?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can we document this?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    We're using this style elsewhere:
    

    

    export class CenteredElementManager extends BaseView<
    ...,
    ...
> {
    ...
}

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  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<...>
      
 
      

      2. 


 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Should we move this into onInitialRender?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can we list the version this will be removed?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 






  9. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Missing docs.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 






  10. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    11. 






  11. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can you add docs for this?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    12. 






  12. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Should be Partial<...>.
    
  
    
 
    

 
    
  



 
    

    13. 




  13. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    14. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  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. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-7.x (5ebe971)





 


 










  

 









     

    

   

  

  

   
   Loading...