On 11/03/2010 02:43 PM, Endi Sukma Dewata wrote:
On 11/3/2010 10:09 AM, Adam Young wrote:
A few questions (and tweaks). Note that I have just given the code a
read through, not applied the patch yet.
Are you sure we want to implement our own Theme code? I'd rather try
to keep theme stuff as part of JQUery.UI. At a mionimum, we risk name
clash and confusion over the term 'theme'.
I think they will complement each other. The jQuery UI theme is
limited to CSS. The IPA theme I'm creating is for the layout. Although
CSS can do layout too, it is limited to the elements and classes that
are already defined in the HTML page. If you need to change the
elements in the HTML page you'd have to change the JavaScript code
because it's generated dynamically. For example, it's not possible to
change this layout:
First Name: Adam
Last Name: Young
into this layout
Last Name: Young First Name: Adam
using CSS alone unless you specify different ID's or assign different
CSS classes for each HTML element. And changing the JavaScript code to
support this specific layout would be either too difficult or not very
useful for anything else.
With the IPA theme you could create 2 different templates:
<dl>
<dt>First Name:</dt>
<dd><span name="firstName"/></dd>
<dt>Last Name:</dt>
<dd><span name="lastName"/></dd>
</dl>
or
<table>
<tr>
<td>Last Name:</td>
<td><span name="lastName"/></td>
<td>First Name:</td>
<td><span name="firstName"/></td>
</tr>
</table>
and not change a single code. The jQuery UI theme can still be applied
on top of this.
Very cool, but suggest we change the term. Would layout perhaps be better?
add.js line 34: Do we really need accesor like this? There is nothing
wrong with doing modifying the member directly. I see the code at line
62 that delegates it down the tree...I think there is a more
javascript-y way to do this. Look up Javascript accessors.
You mean like this? http://offthelip.org/?p=101
Yes, we can do that. Regardless, the accessor is necessary because a
widget may contain a set of other widgets and we want to let the
widget figure out how to pass the value to the other widgets. Ideally
I prefer if we can get rid of the entity_name from widgets, but we can
do that another time.
yeah, like that. I'd rather not use a naming convention that is
different from what the language supports.
If you are going to change a function header like on associate line
133, go ahead and remove the camel_casing as well. (manyObjPKey) as
you seem to be doing variable cleanup elsewhere.
OK, I can fix that too. I had to draw the line somewhere, otherwise
the patch will be too big (it's already bigger than I wanted).
Line 297, executor takes 7 params, that are all member variables of
"that". Since that.execute is invoked as a method, you can remove
these parameters and instead, internal to executor, refer to them via
this.<param>
Not sure that would be a good idea. "that" in this code refers to the
dialog box. The executor is one of the serial/bulk associator.
Associator is not a dialog box, so referring to the parameters using
this might work but will be confusing. This code is actually calling
the associator's constructor and execute it too. I was planning to
create a base class for the associators, but that's for next time.
Yeah. PLus, with the Bulk plugin, we'll want to change the name of the
bulk associator to something more correct, like single_call versus
bulk_call, and change the serial associator to use the bulk plugin.
Typo line 344: that.member_attrribute
OK, I'll fix it. That was from a copy & paste.
Also: remove the buttons for features that we are not going to implement
this time around
from the top of the page: Troubleshoot, Cull Disabled Rules, And the
TEst Rule link under quick links
You can leave Login SVC and Login Svc Groups , those are coming next,
correct?
Let me remove all of them for now and add it back as I implement them.
That way we can cut a release anytime
without having a broken button.
Agreed
Add rule has a rule type field, but no guidance what to fill it in with.
I suspect this should be a select. Without knowing what to put in here,
you can't add a rule. At a minimum, lets put in text 'allow or deny'
OK, I'll add that text. I was planning to do that in a later patch
because we have the same issue with the service name.
Yeah, lets do it now. I had to go top the CLI to figure out what values
to put in here to get it to work. Add a ticket for replacing it with a
select, too.
Note that this failure case doesn't fail very cleanly. There is an error
that shows up in Friebug. Ignore it for now, as I belive my patch for
handling ticket time out fixes this as a side effect.
Add access time seems to be broken. I get 'that.add is not a function'
Yes, I mentioned that in the patch description. That will be done in a
follow up.
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
