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@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
