<!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/27/2011 12:12 PM, Adam Young wrote:
<blockquote cite="mid:4D41A79B.9090801@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
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 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>
<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>
ACKed in IRC by Edewata. Pushed to master<br>
</body>
</html>