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.

--
Endi S. Dewata

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to