<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
On 01/26/2011 04:32 PM, Adam Young wrote:
<blockquote cite="mid:4D4092F9.9050700@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
On 01/26/2011 12:37 PM, Adam Young wrote:
<blockquote cite="mid:4D405BEF.2030409@redhat.com" type="cite">Rebased
on top of origin/master, and made changes. See comments below.
<br>
<br>
<br>
On 01/20/2011 02:48 PM, Endi Sukma Dewata wrote: <br>
<blockquote type="cite">On 1/20/2011 11:10 PM, Adam Young wrote:
<br>
<blockquote type="cite">If you ACK, please don't push, but let
me do so, as it will likely <br>
conflict with other UI work. <br>
</blockquote>
<br>
There is no major issues, just some comments: <br>
<br>
1. The declarative definition is a bit inconsistent. Some
methods like association() takes a spec, but other methods
like facet() takes an object instance. <br>
<br>
association({ <br>
'name': 'netgroup', <br>
'associator': 'serial' <br>
}). <br>
facet( <br>
IPA.search_facet({ <br>
'name': 'search', <br>
'label': 'Search' <br>
}). <br>
</blockquote>
<br>
The difference is for things that are created self contained,
like association, and things like search and details facets that
require additional declaration. We could change the association
call to require creating the association, but not the other way
around. <br>
<br>
Aside: there should be no need to speficy name or label for
search and details. <br>
<br>
<blockquote type="cite"> <br>
2. The diff tool uses the first line of the function to mark
the chunks like this: <br>
<br>
@@ -593,10 +593,7 @@ IPA.permission = function () { <br>
<br>
Having a function name in the first line would make it easier
to read. Compare this definition: <br>
<br>
IPA.permission = function () { <br>
<br>
with this definition: <br>
<br>
IPA.register_entity(function () { <br>
</blockquote>
<br>
Even better, we can use an associative array, and do the two at
once. <br>
<br>
<br>
<blockquote type="cite"> <br>
3. The following lines (webui.js:128-133): <br>
<br>
IPA.start_entities(); <br>
<br>
for (var i=0; i<IPA.entities.length; i++) { <br>
var entity = IPA.entities[i]; <br>
entity.init(); <br>
} <br>
<br>
probably could be combined into a single method: <br>
<br>
IPA.init_entities(); <br>
</blockquote>
Done <br>
<br>
<blockquote type="cite"> <br>
I think this method name will make more sense. <br>
<br>
4. Entity's init_dialogs() probably could be merged into
entity.init(). <br>
</blockquote>
<br>
Done <br>
<blockquote type="cite"> <br>
5. The entity_factories is probably better be named
entity_classes. Factory is usually an object that creates
multiple other objects. The entity 'factory' is really the
entity class which is only instantiated once. <br>
</blockquote>
<br>
Nah, Factory can create only a singe instance. Classes is too
loaded a term. <br>
<blockquote type="cite"> <br>
6. Typo on search.js:258: <br>
<br>
spec.label = spec.lable || IPA.messages.facets.search; <br>
</blockquote>
<br>
Fixed <br>
<br>
<pre wrap=""><fieldset class="mimeAttachmentHeader"></fieldset>
_______________________________________________
Freeipa-devel mailing list
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a>
<a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
Rebased<br>
<pre wrap="">
<fieldset class="mimeAttachmentHeader"></fieldset>
_______________________________________________
Freeipa-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
<br>
</body>
</html>