Summary

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

Review Request #5587 — Created March 4, 2014 and submitted March 10, 2014, 5:18 p.m.

Information

Owner

chipx86

Repository

Djblets

Branch

master

Bugs

Depends On

Commit

2286c2d...

Reviewers

Groups

djblets

People

Description

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

Testing Done

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

Issues

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, …	david	March 7, 2014, 4:12 p.m.
These should be declared in the var statement at the top of the function.	david	March 7, 2014, 4:07 p.m.
One line?	david	March 7, 2014, 4:07 p.m.
This could be chained onto the assignment.	david	March 7, 2014, 4:09 p.m.
Same here.	david	March 7, 2014, 4:09 p.m.
And here.	david	March 7, 2014, 4:09 p.m.
Which would let you assign this in the definition.	david	March 7, 2014, 4:10 p.m.
Same comments here.	david	March 7, 2014, 4:10 p.m.
You get the idea.	david	March 7, 2014, 4:10 p.m.
Since every test function calls this, it should be moved into beforeEach	david	March 7, 2014, 4:11 p.m.
I think the more common pattern for this is: window.Djblets = window.Djblets \|\| {}	david	March 10, 2014, 4:42 p.m.
Could this be _.bind(Backbone.View.prototype.remove, this) ?	david	March 10, 2014, 4:43 p.m.
Elsewhere you replaced this entity with its unicode equivalent. Did you want to do that here?	david	March 10, 2014, 4:43 p.m.
$parentEl is unused here. Can you run jshint on this code?	david	March 10, 2014, 4:44 p.m.
Missing semicolon.	david	March 10, 2014, 4:49 p.m.

Change Summary:

Fixed some missing imports.

Commit:

08abaeba015eb635525ea682ab697d2428c9bfe4

a55a6b2bac3ec6d224a3306bb6131bd437589201

Diff:

Revision 2 (+1503)

Show changes

	djblets/settings.py
	djblets/configforms/forms.py
	djblets/configforms/pages.py
	djblets/configforms/views.py
	djblets/configforms/templates/configforms/config.html
	djblets/configforms/templates/configforms/config_page.html
	djblets/configforms/templates/configforms/config_page_form.html
	djblets/static/djblets/css/config-forms.less
	8 more

djblets/configforms/templates/configforms/config.html (Diff revision 2)

The issue has been dropped. 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()?

chipx86 March 5, 2014, 11:56 p.m.

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.

chipx86 March 7, 2014, 4:13 p.m.

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

djblets/static/djblets/js/configForms/views/listItemView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
These should be declared in the var statement at the top of the function.
```
djblets/static/djblets/js/configForms/views/listItemView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
One line?
```

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

The issue has been dropped. Show all issues

This could be chained onto the assignment.

chipx86 March 7, 2014, 4:13 p.m.

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.

djblets/static/djblets/js/configForms/views/tests/listItemViewTests.js (Diff revision 2)
The issue has been dropped. Show all issues
```
Same here.
```
djblets/static/djblets/js/configForms/views/tests/listItemViewTests.js (Diff revision 2)
The issue has been dropped. Show all issues
```
And here.
```
djblets/static/djblets/js/configForms/views/tests/listItemViewTests.js (Diff revision 2)
The issue has been dropped. Show all issues
```
Which would let you assign this in the definition.
```
1. chipx86 March 7, 2014, 4:13 p.m.
  I want to be more explicit in unit tests anyway.

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

The issue has been dropped. Show all issues

Same comments here.

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

The issue has been dropped. Show all issues

You get the idea.

djblets/static/djblets/js/configForms/views/tests/listViewTests.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Since every test function calls this, it should be moved into beforeEach
```

Change Summary:


Fixed support for icon names. The background property for the buttons was conflicting with the background for the spritesheet.
Moved a call into a forEach for a unit test suite.
Moved some vars to the top of a function.
Combined two unnecessary lines into one line.

Commit:

a55a6b2bac3ec6d224a3306bb6131bd437589201

e7023cf201365b309ba2474188557007d20775ed

Diff:

Revision 3 (+1498)

Show changes

	djblets/settings.py
	djblets/configforms/forms.py
	djblets/configforms/pages.py
	djblets/configforms/views.py
	djblets/configforms/templates/configforms/config.html
	djblets/configforms/templates/configforms/config_page.html
	djblets/configforms/templates/configforms/config_page_form.html
	djblets/static/djblets/css/config-forms.less
	8 more

djblets/static/djblets/js/configForms/base.js (Diff revision 3)

The issue has been resolved. Show all issues

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

djblets/static/djblets/js/configForms/views/listItemView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Could this be _.bind(Backbone.View.prototype.remove, this) ?
```
djblets/static/djblets/js/configForms/views/listItemView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Elsewhere you replaced this entity with its unicode equivalent. Did you want to do that here?
```
djblets/static/djblets/js/configForms/views/listItemView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
$parentEl is unused here. Can you run jshint on this code?
```
djblets/static/djblets/js/configForms/views/tests/listViewTests.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing semicolon.
```

Change Summary:


Made all the changes requested.
Ran jshint, using the same .jshintrc we have on RB. (I'll commit that file separately.)

Commit:

e7023cf201365b309ba2474188557007d20775ed

2286c2d0cf4d13d810b694d787b81f69a24ae017

Diff:

Revision 4 (+1568)

Show changes

	djblets/settings.py
	djblets/configforms/forms.py
	djblets/configforms/pages.py
	djblets/configforms/views.py
	djblets/configforms/templates/configforms/config.html
	djblets/configforms/templates/configforms/config_page.html
	djblets/configforms/templates/configforms/config_page_form.html
	djblets/static/djblets/css/config-forms.less
	9 more

Ship it!

```
Ship It!
```

Status:: Completed