Add an app for managing multi-"page" config forms.




 


  Review Request #5587
  —
  Created March 4, 2014 and submitted 

 















 
  
  
 




 
  

   Information

  

 
 

  

   
   
chipx86


  


  

   
   
Djblets


  


  

   
   
master


  


  

   
   



  


  

   
   



  


  

   
   
2286c2d...


  


 



 
  

   Reviewers

  

 
 

  

   
   
djblets


  


  

   
   



  


 











 

  
  


This makes it easy for a view to provide collections of configuration

pages with a sidebar used for navigation. Each page can have one or more

forms, each of which can be submitted individually.


This is a generalization of the My Account page from Review Board 2.0,

and the Team Admin page from RBCommons. All the JavaScript and CSS were

directly copied from Review Board (with the addition of the two macros

the .less file needs).




 




 

  
  


Ported Review Board 2.0's My Account page to sit on top of this. Worked

without any problems.




 














 

 






 
 
 

  

   

    
   

   
    
    
  • 
     
    
    • 
    
  • 
     
    
    • 
    
  • 
     
    
    • 
    
  • 
     
    
    • 
    
  • 
     
    
    • 
   

  

  

   
    

     Description
     From
     Last Updated
    

   
   



    

     
      
      
I'm not sure I like assigning to the "formView" variable in a loop like this--it seems like if something breaks, …

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
These should be declared in the var statement at the top of the function.

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
One line?

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
This could be chained onto the assignment.

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
Same here.

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
And here.

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
Which would let you assign this in the definition.

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
Same comments here.

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
You get the idea.

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
Since every test function calls this, it should be moved into beforeEach

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
I think the more common pattern for this is: window.Djblets = window.Djblets || {}

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
Could this be _.bind(Backbone.View.prototype.remove, this) ?

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
Elsewhere you replaced this entity with its unicode equivalent. Did you want to do that here?

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
$parentEl is unused here. Can you run jshint on this code?

     		
     
      daviddavid
     
     
      
     
    




    

     
      
      
Missing semicolon.

     		
     
      daviddavid
     
     
      
     
    


   
  

 












  

 








 














 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/configforms/templates/configforms/config.html
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    I'm not sure I like assigning to the "formView" variable in a loop like this--it seems like if something breaks, we could end up with state leaking from one iteration to the next.
    

    How about just removing the variable definition and chaining new {{form.js_view_classs}}(...).render()?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I'm not as worried, because if an error happens in the constructor, it should bubble up to the top,
      
preventing further executions. At the very least, formView would be undefined, which would cause an
      
error as soon as render() is called, and exit the function. We use this pattern in a bunch of files.
      

      I can change it, but feel the result is a lot less readable.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Opting not to wrap construction in parens just to chain. I don't see any risk of leakage anywhere.
      
 
      

      3. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    These should be declared in the var statement at the top of the function.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    One line?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    This could be chained onto the assignment.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I don't want to chain on constructors. It makes the code too ugly, since you have to wrap the whole thing in parens.
      

      Particularly with unit tests, I want to avoid chaining like this, and instead be more explicit, to help with debugging when things go to hell. Construct, then operate.
      
 
      

      2. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Same here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    And here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Which would let you assign this in the definition.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I want to be more explicit in unit tests anyway.
      
 
      

      2. 


 
    


    

 
    

    9. 






  9. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Same comments here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 






  10. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/static/djblets/js/configForms/views/tests/listItemViewTests.js
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    You get the idea.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    11. 






  11. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Since every test function calls this, it should be moved into beforeEach
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    12. 




  12. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    13. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      djblets/static/djblets/js/configForms/base.js
      


       (Diff revision 3)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    I think the more common pattern for this is:
    

    window.Djblets = window.Djblets || {}

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Could this be _.bind(Backbone.View.prototype.remove, this) ?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Elsewhere you replaced this entity with its unicode equivalent. Did you want to do that here?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    $parentEl is unused here. Can you run jshint on this code?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Missing semicolon.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  7. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...