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

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

chipx86
Djblets
master
2286c2d...
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
  1. 
      
  2. djblets/configforms/templates/configforms/config.html (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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. Opting not to wrap construction in parens just to chain. I don't see any risk of leakage anywhere.

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

  4. 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.

  5. Which would let you assign this in the definition.

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

  6. Same comments here.

  7. djblets/static/djblets/js/configForms/views/tests/listItemViewTests.js (Diff revision 2)
     
     
     
     
     
     
     
     
     

    You get the idea.

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

  9. 
      
chipx86
david
  1. 
      
  2. djblets/static/djblets/js/configForms/base.js (Diff revision 3)
     
     
     
     

    I think the more common pattern for this is:

    window.Djblets = window.Djblets || {}
    
  3. Could this be _.bind(Backbone.View.prototype.remove, this) ?

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

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

  6. 
      
chipx86
david
  1. Ship It!

  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Loading...