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

[Adam Young]

On 02/10/2011 01:13 AM, Endi Sukma Dewata wrote:
> 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.

I had mIssed the classes that these things needed.  Added them back in.

>
> 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');

Done.  Good idea/

>
> 3. The indentation of the target_types array in aci.js is inconsistent.
Fixed
>
> 4. The IPA.hidden_widget doesn't seem to be used. Should this be removed?
Gone baby gone
>
> 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.

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

Agreed.  This was actually part of trial and error to get it to work, 
and it didn't need to be there.  Gone.
>
> 7. There are some jslint warnings.
>
Fixed
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0191-4-target-section-without-radio-buttons.patch
Type: text/x-patch
Size: 25815 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110210/2f2f55ea/attachment.bin>