Port PageView to spina.

 






 

 


  Review Request #12937
  —
  Created April 5, 2023 and submitted 

 




















 

  

   Information

  





 
 

  david
 









 
 

  Review Board
 









 
 

  release-6.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This change ports the base PageView class to spina and TypeScript. As

part of this, the code has been moved into the common subdirectory to

match the bundle that it's a part of.


The one change that was required outside of this class was to update the

DiffViewerPageModel to use preinitialize instead of constructor.

The use of the reserved name constructor has bitten us in the past,

and wasn't working anymore now that the base class was an ES6 class.

 








  


 
 

  
    

  • Ran js-tests.
    • 

  • Verified that page views still rendered correctly.
    • 


 







 









 





  

 


 
 

  



 

  
   


    Summary
    ID

   

  
  

   


    

     

      Port PageView to spina.
      
This change ports the base PageView class to spina and TypeScript. As
part of this, the code has been moved into the `common` subdirectory to
match the bundle that it's a part of.

The one change that was required outside of this class was to update the
`DiffViewerPageModel` to use `preinitialize` instead of `constructor`.
The use of the reserved name `constructor` has bitten us in the past,
and wasn't working anymore now that the base class was an ES6 class.

Testing Done:
- Ran js-tests.
- Verified that page views still rendered correctly.

     


    		
    bf63aeed7cb7febfec71b026b87c46c6acc793c9

   



  
 




 













 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
Since this was accessible on the prototype, but isn't really an instance variable, I wonder if we should make this …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Can we work to document all instance variables going forward?

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
We can return super.remove() instead.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Since this has multiple types involved, let's move actionId to its own line, so the :'s don't all run together.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Are you able to just import 'jasmine-core' and get the same results? I've resolved linting locally with just an import …

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 



















 




 




 














 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86



    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/static/rb/js/common/views/pageView.ts
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Since this was accessible on the prototype, but isn't really an instance variable, I wonder if we should make this static and include it in @spina({prototypeAttrs: [...]})?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Will that work with overriding in inherited classes?
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      OK, just tested and it does work as long as the subclass also uses @spina({prototypeAttrs: [...]})
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Oh, that's supposed to inherit. Hmm... That'd be a bug in Spina if it doesn't.
      
 
      

      4. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/static/rb/js/common/views/pageView.ts
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can we work to document all instance variables going forward?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/static/rb/js/common/views/pageView.ts
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    We can return super.remove() instead.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/static/rb/js/common/views/pageView.ts
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Since this has multiple types involved, let's move actionId to its own line, so the :'s don't all run together.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/static/rb/js/common/views/tests/pageViewTests.ts
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Are you able to just import 'jasmine-core' and get the same results? I've resolved linting locally with just an import 'jasmine', and wasn't sure if it'd work the same way.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      No, that doesn't seem to work.
      
 
      

      2. 


 
    


    

 
    

    7. 




  7. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86



    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  







  

   

    Status:
   

   


    Completed

   

  




  

   

    Change Summary:
   

   

    
Pushed to release-6.x (27a193d)