Code review comment for lp:~mzanetti/ubuntu-ui-toolkit/expanding-listitem

Revision history for this message
Zsombor Egri (zsombi) wrote :

So to summarize what we discussed in hangout (+ some additional things to check with design):
For ListItems.Expandable:
- provide property which configures the possibility to collapse the Expandable while clicked on its "header"
- make sure design does not want a down/up pointing chevron to be shown which would indicate there is something more to be shown (as in OptionSelector's case)

ExpandablesColumn name can stay... Not the nicest name however...

ExpandablesListView to be renamed onto UbuntuListView giving room for other enhancements on ListView, and should be moved under Ubuntu.Components.

Additional comments:
====================
36 +
please remove this empty line.

Expandable:
287 + /*!
288 + \internal
289 + Reparent any content to inside the Flickable
290 + */
291 + property alias children: flickableContent.data

Shouldn't this be a default property? It should also be a public one.

ExpandablesListView:
593 + readonly property alias expandedItem: priv.expandedItem

We should have expandedIndex property as well to keep the API in sync with ListView API.

599 + function expandItem(item) {

As ListView creates the items dynamically, we cannot really rely on instance parameters. This function assumes the instance to be expanded is in visible area). What if the instance is re-created due to flicking? The function should take index instead of instance, and should have an option to set the currentIndex as well.
function expandAtIndex(index, moveCurrent, animate), where index is the expandable index, moveCurrent if defined will move the currentIndex/currentItem, and animate drives whether the move should be animated (currentIndex property set) or not (use ListView function to position the list). The last two parameters are optional.

Also there is no example in documentation on how the function should be used.

624 + function collapse() {
A better name for this would be collapseExpanded()

tst_ExpandablesColumn:
Please add a test case where the column has other list items included.

Beside these, I'd recommend to split the MR in 3: Expandable, ExpandablesColumn and for UbuntuListView. Also, thanks for fixing the UbuntuTestCase, but that fix should also come in a separate MR, maybe the base one for all 3 others. This one could also contain the PickerPanel test fix :)

review: Needs Fixing

« Back to merge proposal