On 01/27/2011 12:12 PM, Adam Young wrote:
On 01/26/2011 04:32 PM, Adam Young wrote:
On 01/26/2011 12:37 PM, Adam Young wrote:
Rebased on top of origin/master, and made changes.  See comments below.


On 01/20/2011 02:48 PM, Endi Sukma Dewata wrote:
On 1/20/2011 11:10 PM, Adam Young wrote:
If you ACK, please don't push, but let me do so, as it will likely
conflict with other UI work.

There is no major issues, just some comments:

1. The declarative definition is a bit inconsistent. Some methods like association() takes a spec, but other methods like facet() takes an object instance.

        association({
            'name': 'netgroup',
            'associator': 'serial'
        }).
        facet(
            IPA.search_facet({
                'name': 'search',
                'label': 'Search'
            }).

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.

Aside: there should be no need to speficy name or label for search and details.


2. The diff tool uses the first line of the function to mark the chunks like this:

  @@ -593,10 +593,7 @@ IPA.permission = function () {

Having a function name in the first line would make it easier to read. Compare this definition:

  IPA.permission = function () {

with this definition:

  IPA.register_entity(function () {

Even better, we can use an associative array, and do the two at once.



3. The following lines (webui.js:128-133):

    IPA.start_entities();

    for (var i=0; i<IPA.entities.length; i++) {
        var entity = IPA.entities[i];
        entity.init();
    }

probably could be combined into a single method:

    IPA.init_entities();
Done


I think this method name will make more sense.

4. Entity's init_dialogs() probably could be merged into entity.init().

Done

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.

Nah, Factory can create only a singe instance. Classes is too loaded a term.

6. Typo on search.js:258:

    spec.label = spec.lable ||  IPA.messages.facets.search;

Fixed


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


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


_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
ACKed in IRC by Edewata.  Pushed to master
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to