ubuntu-touch-coreapps-reviewers team mailing list archive

[Andrew Hayzen <ahayzen@xxxxxxxxx>]

Review: Needs Fixing

A few inline comments, visually ...
1) there appears to be a space between the detected location and the first selected location
2) if you multiselect, select an item, then select delete you are still in multiselect mode, this is weird if you remove the last item in the list as you have to hit back what feels like twice. I suggest when delete is pressed in the header that the multiselect mode is cancelled. 

Diff comments:

> 
> === modified file 'app/components/PageWithBottomEdge.qml'
> --- app/components/PageWithBottomEdge.qml	2015-06-18 01:45:57 +0000
> +++ app/components/PageWithBottomEdge.qml	2015-08-05 00:17:51 +0000
> @@ -77,7 +77,7 @@
>      property bool reloadBottomEdgePage: true
>  
>      readonly property alias bottomEdgePage: edgeLoader.item
> -    readonly property bool isReady: ((bottomEdge.y === 0) && bottomEdgePageLoaded && edgeLoader.item.active)
> +    readonly property bool isReady: ((bottomEdge.y === fakeHeader.height) && bottomEdgePageLoaded && edgeLoader.item.active)

Should this have // CUSTOM as a note incase we do a pull of upstream and miss that we made changes?

>      readonly property bool isCollapsed: (bottomEdge.y === page.height)
>      readonly property bool bottomEdgePageLoaded: (edgeLoader.status == Loader.Ready)
>  
> 
> === modified file 'app/ui/LocationsPage.qml'
> --- app/ui/LocationsPage.qml	2015-07-29 21:34:07 +0000
> +++ app/ui/LocationsPage.qml	2015-08-05 00:17:51 +0000
> @@ -18,40 +18,58 @@
>  
>  import QtQuick 2.4
>  import Ubuntu.Components 1.2
> -import Ubuntu.Components.ListItems 0.1 as ListItem
> +import Ubuntu.Components.ListItems 1.0 as ListItems
>  import "../components"
> -import "../components/ListItemActions"
> -
>  
>  Page {
>      id: locationsPage
>      objectName: "locationsPage"
> +
>      // Set to null otherwise the first delegate appears +header.height down the page
>      flickable: null
>      title: i18n.tr("Locations")
>  
> -    state: locationsListView.state === "multiselectable" ? "selection" : "default"
> +    state: "default"
>      states: [
>          PageHeadState {
> -            id: defaultState
>              name: "default"
> +            head: locationsPage.head
>              actions: [
>                  Action {
>                      iconName: "add"
> -                    onTriggered: mainPageStack.push(Qt.resolvedUrl("AddLocationPage.qml"))
> +                    onTriggered: {

Why was this changed it looks the same just with  { } around it?

> +                        mainPageStack.push(Qt.resolvedUrl("AddLocationPage.qml"))
> +                    }
>                  }
>              ]
> -            PropertyChanges {
> -                target: locationsPage.head
> -                actions: defaultState.actions
> +            backAction: Action {
> +                iconName: "down"
> +                onTriggered: {
> +                    pageStack.pop()
> +                }
>              }
>          },
> -        MultiSelectHeadState {
> -            listview: locationsListView
> -            removable: true
> -            thisPage: locationsPage
> -
> -            onRemoved: storage.removeMultiLocations(selectedItems.slice());
> +        PageHeadState {
> +            name: "selection"
> +            head: locationsPage.head
> +            when: locationsListView.ViewItems.selectMode
> +            backAction: Action {
> +                iconName: "back"
> +                onTriggered: {
> +                    locationsListView.closeSelection()
> +                    locationsPage.state = "default"
> +                }
> +            }
> +            actions: [
> +                Action {
> +                    iconName: "delete"
> +                    enabled: locationsListView.ViewItems.selectedIndices.length !== 0
> +                    onTriggered: {
> +                        storage.removeMultiLocations(locationsListView.ViewItems.selectedIndices)
> +                        locationsListView.clearSelection()
> +                    }
> +                }
> +            ]
>          }
>      ]
>  
> @@ -59,27 +77,29 @@
>          id: currentLocationModel
>      }
>  
> +    ListModel {
> +        id: locationsModel
> +    }
> +
>      MultiSelectListView {
>          id: locationsListView
> -        anchors {
> -            fill: parent
> -        }
> -        model: ListModel {
> -            id: locationsModel
> -        }
> +
> +        clip: true
> +        anchors.fill: parent
> +        model: locationsModel
> +
>          header: MultiSelectListView {
>              id: currentLocationListView
> -            anchors {
> -                left: parent.left
> -                right: parent.right
> -            }
> +
> +            width: parent.width

Not A->Z and with spacing?

>              height: settings.addedCurrentLocation && settings.detectCurrentLocation ? units.gu(8) : units.gu(0)
>              interactive: false
>              model: currentLocationModel
> -            delegate: WeatherListItem {
> +
> +            delegate: ListItem {
>                  id: currentLocationListItem
>  
> -                onItemClicked: {
> +                onClicked: {
>                      settings.current = index;
>                      pageStack.pop()
>                  }
> @@ -194,6 +225,7 @@
>  
>                      Label {
>                          id: locationName
> +                        width: parent.width

not A->Z

>                          elide: Text.ElideRight
>                          fontSize: "medium"
>                          text: name
> @@ -204,6 +236,7 @@
>                          elide: Text.ElideRight
>                          fontSize: "small"
>                          font.weight: Font.Light
> +                        width: parent.width

not A->Z

>                          text: adminName1 == name ? countryName : adminName1
>                      }
>                  }


-- 
https://code.launchpad.net/~ubuntu-weather-dev/ubuntu-weather-app/reboot-finish-listitem-migration/+merge/266981
Your team Ubuntu Weather Developers is subscribed to branch lp:~ubuntu-weather-dev/ubuntu-weather-app/reboot-finish-listitem-migration.