[Freeipa-devel] [PATCH] admiyo-0221-action-panel-to-top-tabs

[Endi Sukma Dewata]

On 4/26/2011 12:42 PM, Adam Young wrote:
>>>>>> 20. The facet groups needs to be customizable. Currently it's stored
>>>>>> in unordered set in entity's facet_group and the order is hard-
>>>>>> coded in get_facet() and facet_tabs(). It would be difficult
>>>>>> to workaround this limitations in custom facets.
>>>>>>
>>>>>> See how facets are stored in entity. The names are stored in an
>>>>>> ordered list (that.facets) and the instances are stored in a
>>>>>> dictionary (that.facets_by_name). Facet groups should use a similar
>>>>>> method to store the order and facet collections.
>>>>>>
>>>>>> In get_facet() the default facet should be the first facet in the
>>>>>> first group. In facet_tabs() the tab_section should be created
>>>>>> according to the order they are stored.
>>>>>
>>>>> Agreed, but we have this issue now, and so is not a regression. We'll
>>>>> fix in a follow on patch.
>>>
>>> This is actually a regression. Because of this the Host's and
>>> Service's Managed By is missing. We also want to be able to replace
>>> the facet group names in ACI because they don't quite make sense
>>> (e.g. Role being a member of Privilege) although that's how it's
>>> implemented internally.

As discussed over IRC, patch 221-7 still includes hard-coded facet group 
names. It may not be an issue for IPA now, but it will be for other 
projects trying to use the framework.

>>>>>> 21. The IPA.entity_header should be created in entity.init()
>>>>>> instead of
>>>>>> in entity.setup() line 305. The entity.setup() only needs to store
>>>>>> the container in the entity.header. This also eliminates manual
>>>>>> creations in the qunit tests.
>>>>>
>>>>> Long term, agreed, but init doesn't have the container, so under the
>>>>> current approach, we can't put it there.
>>>
>>> It can be done with a simple change. The following code in the
>>> IPA.entity_header can be moved into a create() method which takes a
>>> container parameter:
>>>
>>> that.header = $("<div class='entity-header'/>").
>>> append(title(entity)).
>>> append(buttons()).
>>> append(pkey()).
>>> append(search_bar()).
>>> append(entity_container());
>>> container.append(that.header);
>>>
>>> The IPA.entity_header should be created together with the entity. It
>>> doesn't need a container. Then the create() method can be called by
>>> IPA.entity_setup(). This way a custom entity can customize the
>>> entity_header.
>>>
>>> It's also better to append the DOM object as soon as it's created
>>> instead of appending a complex object into the container. It helps
>>> troubleshooting.

This is not addressed, but it's not a big issue.

>>> 22. When hovering above the facet tabs, the mouse changes into a text
>>> cursor instead of pointer.

Not addressed, but not a big issue.

>>> 23. Facet tab can change although the page is dirty.
>>> Open Users tab, click one of the users, this will open the Settings
>>> facet. Edit a field, then click another facet, the Dirty dialog box
>>> will appear. Close the dialog box, the tab doesn't change back to
>>> Settings.

Not addressed, but not a big issue.

> Merged in patch 222, since this patch does not work with out it.

24. Typo in entity.js and search.js: back_to-seach
25. Typo in ipa.css: .entity-continer
26. Incorrect jQuery selector in search.js: entity-tabs.li
27. There's a jslint warning.

Feel free to push if the remaining issues can be fixed before pushing or 
considered minor.

-- 
Endi S. Dewata