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
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
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:
and not change a single code. The jQuery UI theme can still be applied
on top of this.
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
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
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
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.
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,
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.
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.
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
Endi S. Dewata
Freeipa-devel mailing list