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

  

 
 

  


   

    
     
    

   

   
Port PageView to spina.


  


 






 




 

 






 
 
 

  

   

    
   

   
    
    
  • 
     
    
    • 
    
  • 
     
    
    • 
    
  • 
     
    
    • 
    
  • 
     
    
    • 
    
  • 
     
    
    • 
   

  

  

   
    

     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. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    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. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    We can return super.remove() instead.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    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:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-6.x (27a193d)





 


 










  

 









     

    

   

  

  

   
   Loading...