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.

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.

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.

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.

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 follow up.

--
Endi S. Dewata

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to