[Freeipa-devel] [PATCH] admiyo-0191-target-section-without-radio-buttons

[Endi Sukma Dewata]

On 2/9/2011 7:06 PM, Adam Young wrote:
>

A few comments:

1. The functionality seems to be working, but the layout is a bit 
different. Previously the label (e.g. Filter) and the widget (e.g. text 
field) occupy the same line. Right now they occupy different lines and 
not aligned with the labels & widgets above it (e.g. Permission name). 
I'd like the UXD team to review this change.

2. The jQuery selectors on lines 427, 462, 472 in aci.js are not 
qualified, so they will be doing a global search. I'd rather store the 
object reference somewhere and use it directly without searching for it 
again. For example, line 411 can be changed as follows:

   target_type.container = $('<dl/>', {

Then line 427 can be changed as follows:

   target_type.container.css('display', 'block');

3. The indentation of the target_types array in aci.js is inconsistent.

4. The IPA.hidden_widget doesn't seem to be used. Should this be removed?

5. For the changes in dialog.js, it's not necessary to check 
section.reset()'s presence before calling it. All sections will have a 
reset() function because it's inherited from the base class.

6. For the changes in widget.js, let's do this in a separate patch. 
We'll combine the create/setup in a more consistent way.

7. There are some jslint warnings.

-- 
Endi S. Dewata