<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> <html> <head> <meta content="text/html; charset=UTF-8" http-equiv="Content-Type"> <title></title> </head> <body bgcolor="#ffffff" text="#000000"> 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: <blockquote cite="mid:4C8EDCF2.6080206@redhat.com" type="cite"> <meta content="text/html; charset=UTF-8" http-equiv="Content-Type"> 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: <div class="pre">this.setup = function(facet){</div> <div class="pre"><a moz-do-not-send="true" id="l100" href="http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l100" class="linenr"> </a> if (this[facet]){</div> <div class="pre"><a moz-do-not-send="true" id="l101" href="http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l101" class="linenr"> </a> this[facet].setup();</div> <div class="pre"><a moz-do-not-send="true" id="l102" href="http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l102" class="linenr"> </a> }else{</div> <div class="pre"><a moz-do-not-send="true" id="l103" href="http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l103" class="linenr"> </a> this.unspecified.setup();</div> <div class="pre"><a moz-do-not-send="true" id="l104" href="http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l104" class="linenr"> </a> }</div> <div class="pre"><a moz-do-not-send="true" id="l105" href="http://git.fedorahosted.org/cgi-bin/gitweb.cgi#l105" class="linenr"> </a> }</div> 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 <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> </body> </html>