[Freeipa-devel] [PATCH] 0270-removing-setters-setup-and-init

[Adam Young]

Last patch introduced another JSL error with the use of true and false 
as object properties.  This version access them as strings.

On 07/22/2011 07:24 PM, Endi Sukma Dewata wrote:
> On 7/22/2011 12:18 PM, Endi Sukma Dewata wrote:
>> On 7/22/2011 10:34 AM, Adam Young wrote:
>>> While this version has a Navigation fix in it, it is going to be 
>>> rebased
>>> on top of Endi Dewata's patch for putting the state inside each of the
>>> facets.
>>
>> Rebased. I'll continue the review.
>
> Some issues:
>
> 30. The IPA.spacer_widget is used to create spacing for layout. 
> Widgets should not be used this way because it diverges from the 
> original design. Widgets are supposed to correspond to attribute. It 
> has a name (required for storing in a map), metadata, load/save/undo 
> methods, none of which is relevant for spacer. Also, it only handles a 
> narrow case for creating a space between 2 fields which I think better 
> be handled using sections. This should be addressed in a separate patch.
>
> 31. The create() should only be used to create the visual elements. 
> Creating fields, columns, etc. should not be done inside create() 
> because we might decide to destroy the HTML elements of hidden facets 
> and recreate it again. In that case the fields will be duplicated. 
> Also, by delaying the creation of the fields it makes the object 
> incomplete after creation, which is the reason for removing init().
>
> 32. This is an existing problem but it's worsening without init(). 
> Since there is no class constructor, the initialization code is 
> scattered throughout the class, making it difficult to maintain. Some 
> code needs to be written in certain position, but in general should we 
> move the initialization code to the bottom of the class (to make sure 
> all methods are already defined)? Or should init() be used as a 
> constructor and called at the bottom of the class?
>
> 33. In association.js:661 the spec object is referenced without 
> ensuring it's not null. The code below it supposed to do that.
>
> 34. Commented code in entity.js:746 can be removed.
>
> 35. Commented code in widget_tests.js:181 can be removed.
>
> 36. The widget_create() is called twice in widget.js:1597.
>
> 37. Untranslated True/False labels in HBAC and sudo.
>
> 38. The file install/ui/rule.js~ got included.
>
> 39. There are jslint warnings.
>
> 40. The selenium tests could be fixed in separate patch and probably 
> pushed earlier. I have not run them.
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0270-11-removing-setters-setup-and-init.patch
Type: text/x-patch
Size: 189668 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110725/849ed536/attachment.bin>