Port FileAttachmentThumbnailView to TS/spina.

 






 

 


  Review Request #13221
  —
  Created Aug. 17, 2023 and submitted 

 




















 

  

   Information

  





 
 

  maubin
 









 
 

  Review Board
 









 
 

  release-6.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This ports the FileAttachmentThumbnail view to TS/spina. The view is also

renamed to FileAttachmentThumbnailView to follow our naming conventions.

 








  


 
 

  
    

  • Ran JS unit tests.
    • 

  • Tested using/viewing file attachment thumbnails on the review request,
    
  in change descriptions, and in file review UIs.
    • 

  • Tested uploading/deleting files.
    • 

  • Tested all the file actions.
    • 


 







 









 





  

 


 
 

  



 

  

    

  
  
   

     Summary

   

  
  

   


    
Port FileAttachmentThumbnailView to TS/spina.


   


  
 




 













 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
The summary and description need to be separated by a blank line.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
I don't know that we've standardized this yet for TypeScript, but we should probably have required fields before optional fields. …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
While here, can you turn this (and others) into a dedent, so we can save on the join?

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Worth mentioning, we'll do a $() for every one of these when constructing the object (since these are instance attributes, …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Let's organize _ before #, since _ is semi-public and # is semi-private. We haven't done this elsewhere, but we …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
onInitialRender() shouldn't return anything. If it did, we'd actually want to use this instead of the class name as the …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Missing a trailing comma.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Missing a trailing comma.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
ESLint will complain about the ordering here. This should be in alphabetical order.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Let's define the types for these here.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Since we're touching this, can we change var to const, add a trailing comma on canEdit, and sort the object …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
These default values can all be specified in the instance definitions.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Parens around val aren't necessary here.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
If you pull, you can now add: import { suite } from '@beanbag/jasmine-suites';

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Just realized, since we renamed FileAttachmentThumbnail to FileAttachmentThumbnailView, we should at least document the name in a Version Changed, and …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
There's a section header for this: /** * ... * * Deprecated: * 6.0 */ When we finally have generated …

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 



















 




 




 














 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    The summary and description need to be separated by a blank line.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    I don't know that we've standardized this yet for TypeScript, but we should probably have required fields before optional fields. This better matches the requirements imposed in Python for dataclasses, and how we often document things.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    While here, can you turn this (and others) into a dedent, so we can save on the join?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Worth mentioning, we'll do a $() for every one of these when constructing the object (since these are instance attributes, not class attributes -- silly ES6 classes).
    

    This is fine if we want that as the default, but since these are set in render, it might be a better failure to leave these as undefined, at least in some cases?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Let's organize _ before #, since _ is semi-public and # is semi-private.
    

    We haven't done this elsewhere, but we should probably also mark the _ ones with the private keyword, unless we need them in tests.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    onInitialRender() shouldn't return anything.
    

    If it did, we'd actually want to use this instead of the class name as the type.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Missing a trailing comma.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 






  9. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Missing a trailing comma.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 






  10. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    ESLint will complain about the ordering here. This should be in alphabetical order.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Woops, this made me realize that I didn't have ESLint set up.
      
 
      

      2. 


 
    


    

 
    

    11. 




  11. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    12. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Let's define the types for these here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/templates/reviews/changedesc_file_attachment.html
      


       (Diff revision 3)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Since we're touching this, can we change var to const, add a trailing comma on canEdit, and sort the object keys here?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    These default values can all be specified in the instance definitions.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    If you pull, you can now add:
    

    

    import { suite } from '@beanbag/jasmine-suites';

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




david




   

   









  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Parens around val aren't necessary here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Change looks ready to go. Just one thing I want to make sure we have an answer for.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Just realized, since we renamed FileAttachmentThumbnail to FileAttachmentThumbnailView, we should at least document the name in a Version Changed, and if there's any reason extensions ever needed to refer to this by name (I don't think so but maybe?) then we'd want an alias from the old name to new name.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Makes sense. For the alias, could I achieve that by doing this in the fileAttachmentThumbnailView.ts file?
      

      @spina
export class FileAttachmentThumbnail extends FileAttachmentThumbnailView {}

      

      

      (and export it from the index.ts too)
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      You could do it in the root index.ts by doing:
      

      

      /* Some docs mentioning this alias and marking it deprecated */
export const FileAttachmentThumbnail = FileAttachmentThumbnailView;

      

      

      Or it could be done here as well with a similar assignment, and then funnel that down.
      
 
      

      3. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 





 

  

   




chipx86




   

   









  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    There's a section header for this:
    

    

    /**
 * ...
 *
 * Deprecated:
 *     6.0
 */

    

    

    When we finally have generated docs for this, that'll show up with a deprecation notice.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




maubin




   

   

  

 



 















 



 

  

   




maubin




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-6.x (073851e)





 


 










  

 









     

    

   

  

  

   
   Loading...