Added Collapse All button

Review Request #2808 — Created Jan. 21, 2012 and submitted

Information
medanat
Review Board
Reviewers
reviewboard 
Added permanent Collapse All button instead of a dynamic toggle.
Local machine: Ubuntu/Chromium.
Description From Last Updated

I think we're going to be putting the issue summary table into the details box above the series of review …

 mike_conleymike_conley

Just curious - why aren't we using the alt attributes?

 mike_conleymike_conley
SM
smacleod 
  1. The code repetition makes me cringe a little, not sure how to fix it though.
  2. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) 
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    I feel like this should be collapsed into a single function, but can't think of a good way to do it. The javascript might be simpler if you used hiding instead of modifying the html. Although, that would just be creating repetition in the html instead...
    1. Would it make sense to show both "Expand All" and "Collapse All" buttons.
I can see cases where I might have a couple of them collapsed but wish to expand all of them (and it might not be entirely intuitive to click "Collapse All" before clicking "Expand All".
    2. Anthony's got a good point.  Can we try this with both the Expand All and Collapse All buttons visible?
  3. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   





 ME





   

   

  

 



 















 





 

  

   




mike_conley




   

   







  

 



 

  


   

   

    
 
mike_conley


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Yazan:

This looks good.  Good work.  Two questions.  Once resolved...I think you've got my ship-it.

-Mike
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    I think we're going to be putting the issue summary table into the details box above the series of review boxes...so I'm not sure this will apply...
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Since that is the current behaviour of the page, I believe the comment is valid. The current implementation expands and collapses any "box" class element using the generic "collapse" class. If the layout changes, then the implementation of these functions will change along with the comment.
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Just curious - why aren't we using the alt attributes?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      This was the state of the img element before the edit. I do not know why the alt attribute was left empty, or why the CSS was inline. I can fix both issues if there is no real reason for this implementation.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      It's empty because the <a> also has the text. If it was there, screen readers would read this as "Expand All Expand All".
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      That makes absolute sense. Thanks David. Is there a reason why the styling was done inline?
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      I'm not sure why border was included here instead of the stylesheet. The width and height attributes are used for layout.
      
 
      

      5. 



    5. 
 
 
      
 
      
  
 
      
 
      
  
      I think the issues are resolved? I'll keep the border style inline for consistency, and I'll change the comment if the implementation changes.
      
 
      

      6. 


 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   




mike_conley




   

   







  

 



 

  


   

   

    
 
mike_conley


    
   


  

  

  



    




  1. 
 
    
  
    
   
    This looks good to me.  Thanks Yazan! Great work!

-Mike
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 ME





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to master (b801d96). Thanks!





 


 










  

 









     

    

   

  

  

   
   Loading...