ubuntu-touch-coreapps-reviewers team mailing list archive

[Andrea Bernabei <andrea.bernabei@xxxxxxxxxxxxx>]

just a couple of comments, looks very good otherwise ;) thanks!

Diff comments:

> === modified file 'app/components/ExpandableListItem.qml'
> --- app/components/ExpandableListItem.qml	2016-02-25 22:16:54 +0000
> +++ app/components/ExpandableListItem.qml	2016-03-02 18:33:57 +0000
> @@ -33,46 +33,46 @@
>      property Component delegate
>      property alias titleText: expandableHeader.title
>      property alias subText: expandableHeader.subtitle
> -    property alias listViewHeight: expandableList.height
> +    property alias listViewHeight: expandableListLoader.height
>  
> -    height: expandableHeader.height + divider.height
> -    expansion.height: contentColumn.height
> +    height: headerListItem.height
> +    expansion.height: headerListItem.height + expandableListLoader.height
>      onClicked: expansion.expanded = !expansion.expanded
>  
> -    Column {
> -        id: contentColumn
> -
> -        anchors {
> -            left: parent.left
> -            right: parent.right
> -        }
> -
> -        ListItem {
> -            height: expandableHeader.height + divider.height
> -            ListItemLayout {
> -                id: expandableHeader
> -
> -                Icon {
> -                    id: arrow
> -
> -                    width: units.gu(2)
> -                    height: width
> -                    SlotsLayout.position: SlotsLayout.Trailing
> -                    SlotsLayout.overrideVerticalPositioning: true
> -                    anchors.verticalCenter: parent.verticalCenter
> -                    name: "go-down"
> -                    rotation: expandableListItem.expansion.expanded ? 180 : 0
> -
> -                    Behavior on rotation {
> -                        UbuntuNumberAnimation {}
> -                    }
> +    ListItem {
> +        id: headerListItem
> +        height: expandableHeader.height + divider.height
> +
> +        ListItemLayout {
> +            id: expandableHeader
> +
> +            Icon {

please doublecheck with a visual designer that this is what they want. Maybe they want an exception for vertical positioning for this case (that is why I previously asked you if the vertical centering was dictated by design ;) )

> +                id: arrow
> +
> +                width: units.gu(2)
> +                height: width
> +                SlotsLayout.position: SlotsLayout.Trailing
> +                name: "go-down"
> +                rotation: expandableListItem.expansion.expanded ? 180 : 0
> +
> +                Behavior on rotation {
> +                    UbuntuNumberAnimation {}
>                  }
>              }
>          }
> -
> +    }
> +
> +    Loader {

I believe this is going to create UI lag on weak devices, can you try with async set to true?
Note: at that point you also have to doublecheck that the binding on height is working as expected even when it takes some time to instantiate the loader of the component :) (it could be the case, for instance, that the height doesn't update after the async loading has completed)

> +        id: expandableListLoader
> +        width: parent.width
> +        anchors.top: headerListItem.bottom
> +        sourceComponent: expandableListItem.expansion.expanded ? expandableListComponent : undefined
> +    }
> +
> +    Component {
> +        id: expandableListComponent
>          ListView {
>              id: expandableList
> -            width: parent.width
>              interactive: false
>              model: expandableListItem.model
>              delegate: expandableListItem.delegate


-- 
https://code.launchpad.net/~nik90/ubuntu-clock-app/dynamic-load-listview/+merge/287831
Your team Ubuntu Clock Developers is requested to review the proposed merge of lp:~nik90/ubuntu-clock-app/dynamic-load-listview into lp:ubuntu-clock-app.