[Freeipa-devel] [PATCH] Big webUI patch.

[Adam Young]

Feedback:


First of all, let me say that this is a tremendous effort.  I'm 
impressed.  Lots of good work here.

Don't include the full state of the application, just the current tab.  
The URL gets too long, and the application becomes confused.  When 
transitioning betwen tabs, if you want to keep the state of other tabs, 
save the whole pre hashchange state in a hashtable, keyed on the tab 
name.  It won't be bookmarked, but it will live as long as the user 
doesn't do a reload.

Facets are specific to the entity, not a generalizable list.  The code 
that manages the set of facets should take a list from the entity 
object.  Take a look at how the most recent netgroup.js file manages them:

this.setup = function(facet){
<http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l100>         
if (this[facet]){
<http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l101>             
this[facet].setup();
<http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l102>         }else{
<http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l103>             
this.unspecified.setup();
<http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l104>         }
<http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l105>     }

But we also maintain an array:   
this.facets = ["details","users","groups","hosts","hostgroups"];

(I've removed the assign<entity> factets, as they are going to be modals 
just like 'add' is for 'details')

This is one place where JavaScript falls down.  We should be able to 
enumerate through the property names of the object, but JavaScript 
enumeration does not honor order.

The CSS is broken and needs to be redone for:
     toplevel tabs
     subtabs
     facets
     list tables

As you mentioned, we need to add services back in.

I get an error on an undefined variable  associationsList.  Need to hunt 
that code down and remove it.


In navigation.js

      I'm not a fan of the template approach.  I prefer the $jquery way, 
as it at least validates the nodes.  Please replace code like


var _nav_li_tab_template = '<li><a href="#I">N</a></li>';

function nav_insert_tab_li(jobj, id, name)
{
     jobj.append(_nav_li_tab_template.replace('I', id).replace('N', name));
}


with $("<li>" {
html = $("<a/>",{
     id=id,
     href=name
}




Don't prepend functions with ipa_ or nav_.  We should not be creating 
new global variables.  Instead, create a single global var ipa= {};

And then the global variables and functions go under that as:

ipa.entity={
     search_list: {};

     set_search_definition: function(obj_name, data)
     {
         search_list[obj_name] = data;
     }

     function tab_on_click(obj)
     {
         var jobj = $(this);
         var state = {};
         var id = jobj.closest('.tabs').attr('id');
         var index = jobj.parent().prevAll().length;

         state[id] = index;
         $.bbq.pushState(state);
     }

}

functions like tab_on_click that you want to be local should not be 
exposed in the public interface, just leave them like this and the other 
members of ipa.entity have access to them, but nothing else.


Don't repeat long parameter lists.  Create a spec object, and pass it in 
to the functions.  thus:

function nav_create(nls, container, tabclass)

becomes
/ *spec must have nls, container, tabclass*/
function nav_create(spec);

Then it can be called

nav_create({nls : blah, container : that, tabclass: "tabclass"});

Ideally this is done for factories and Constructors.


webui.js has the javascript function that kicks off all of the loigic, 
but it might get executed too early.  It gets executed when the webui.js 
file is parsed, which might be before the index.xhtml file is fully 
loaded.  It doesn't seem to be a problem, but one way to make sure is to 
put it at the end of the index.xhtml file, or to put an onload event 
hander lin the index.xhtml file that then calls the code in 
webui.xhtml.  It is OK to start JS processing prior to the load of the 
main page, so long as it doesn't modify the dom of the main page.  I 
suspect that the reason this works so far is  because of the additional 
json calls for init and for whoami.

Again, delegate the code of the form "if (facet)..." to the tab  object, 
just like the setup code above.

add.js: Add/ Add Edit should be Add and Edit /Add and Add Another.  The 
logic looks OK, just the labels are off, I think

associate.js : The H1 tag is rendereing both above and below the 
enrollments.

We should change obj_name to entity, but not in this patch.

groups.js:  f_posix should probably be if_posix










-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20100913/4bb4d5f2/attachment.htm>