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

[Adam Young]

Of all this feedback, the only things I consider necessary prior to a 
checkin are:

CSS
Facet list

Everything else can wait, I just wanted to get the full analysis recorded.



On 09/13/2010 10:24 PM, Adam Young wrote:
> 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
>
>
>
>
>
>
>
>
>
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

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