Review Request Drop Down U.I. Extension




 


  Review Request #2224
  —
  Created March 30, 2011 and submitted

 















 
  
  
 




 
  

   Information

  

 
 

  

   
   
kfquinn


  


  

   
   
Review Board


  


  

   
   



  


  

   
   



  


  

   
   



  


 



 
  

   Reviewers

  

 
 

  

   
   
reviewboard


  


  

   
   
mike_conley


  


 











 

  
  


I needed a drop down menu for an extension I was building so I built an extension to do just that.  Very similar to the standard button extension, except it's a list of buttons essentially.




 




 

  
  


Manual testing using the extension I made.




 














 

  
  


   

    

    

     

    

   


   

  

 


 














  

 








 














 



 

  

   





 KF





   

   

  

 



 















 





 

  

   




mike_conley




   

   





  

 



 

  


   

   

    
 
mike_conley


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Kevin:

This looks pretty good.  Two really minor comments below.

I think ChipX86 or purple_cow should give this a once-over before it get's shipped - we're reusing some code here, and it might be more awkward than they'd like.

Thanks,

-Mike
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/extensions/hooks.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    I think this is a little confusing/misleading.  Developers might try to set options as a list of ActionHook objects.

Maybe say, "a list of ActionHook-style dicts".

I'm not sure if that's any clearer.  :/
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
    space before "actions"
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/extensions/hooks.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    The comment should be more like the one above. It should be clear from reading it exactly what it's purpose is.

First line, on the """, in the form of a quick summary without "This is" or similar.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/extensions/hooks.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    Shouldn't have so many spaces after the :
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/extensions/hooks.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    I'm also confused about this. Also the "see above." What should be in here? Is it actually ActionHook instances?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Options is a dict of action hooks.  The parameters are the same.
      
 
      

      2. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    




    
    One line.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      But.. every other method is commented in this fashion, should I change all of them?
      
 
      

      2. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    




    
    This must wrap to < 80 characters. Probably want one parameter per line.

Also, spaces after ",".
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    




    
    The image path is being hard-coded and doesn't contain a media serial. Please use:

{{MEDIA_URL}}rb/images/dropdown.png?{{MEDIA_SERIAL}}
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I tried this, but it doesn't seem to know {{MEDIA_URL}} & {{MEDIA_SERIAL}} is.
      
 
      

      2. 


 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 

   
    
    
    




    
    No spaces before this. Instead, put spaces after the {% for indenting.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  9. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 




  

 
















 



 

  

   





 KF





   

   

  

 



 















 



 

  

   





 KF





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
Style changes, adding example, using {{settings.MEDIA}}, commenting changes.





 



  

   
Diff:

   
    
 Revision 3 (+56 -3)
Show changes
 
    reviewboard/extensions/hooks.py 
    reviewboard/extensions/templatetags/rb_extensions.py 
    reviewboard/templates/extensions/action_dropdown.html 
    reviewboard/templates/reviews/review_detail.html 
 

   		
  



 










  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Committed with a few style fixes. Thanks!

There are a few small question marks in my head about the bet way to implement this dropdown actionhook. I almost want a standard ActionHook to be able to supply sub-items. Also, I sort of think it should be more generic so it can be used in the diff viewer too. However, it's fine for now, and I think it's very likely we'll be revisiting how actionhooks work by the time we ship extensions in a release.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 









     

    

   

  

  

   
   Loading...