ubuntu-touch-coreapps-reviewers team mailing list archive

[Nekhelesh Ramananthan <krnekhelesh@xxxxxxxxx>]

Review: Needs Fixing code-review

Nice. The switch to ListItemLayout removes some code. I see more opportunities to remove code. Let me list those along with some fixes which are still needed.

1. In the code below you have added 2 MouseArea which is set to fill the whole ListItem. However ListItems come with their MouseArea by default. We don't need to create a additional one. So the code below can be simplified into,

==> onClicked: {
         Haptics.play()
         pageStack.push(Qt.resolvedUrl("EventDetails.qml"), {"event":event,"model":eventListModel});
    }

==> onClicked: {
         Haptics.play()
         dateSelected(event.startDateTime);
    }

+                    // MouseArea, onClicked open new edit event page.
+                    MouseArea {
+                        id: detailsMouseArea
+                        anchors.fill: parent
                         onClicked: {
+                            Haptics.play()
                             pageStack.push(Qt.resolvedUrl("EventDetails.qml"), {"event":event,"model":eventListModel});
                         }
}

+                    // MouseArea, onClicked new page with dayle view will open.
+                    MouseArea {
+                        id: headerMouseArea
+                        anchors.fill: parent
+                        onClicked: {
+                            Haptics.play()
+                            dateSelected(event.startDateTime);
+ }

2. I see that you created 2 Rectangles below to fill the ListItem. This again isn't necessary. We can set the ListItem background color and highlightColor directly. The highlightColor will be used when a ListItem is pressed *provided* the listitem has its onClicked connected to a function, which we did in step 1 :).

Creating additional MouseArea & Rectangles are expensive especially when it comes to a ListItem being used in ListView. We need to be very careful here.

===> color: "#F7F7F7"
     highlightColor: "#EDEDED"

+                    // ListItem background (changing color when pressed)
+                    Rectangle {
+                        anchors.fill: parent
+                        color: headerMouseArea.pressed ? "#ededed" : "#f7f7f7"
+ }

+                    // ListItem background (changing color when pressed)
+                    Rectangle {
+                        anchors.fill: parent
+                        color: detailsMouseArea.pressed ? "#f7f7f7" : "white"
}

3. Also about these two labels, 

+ timeLabelStart.text = startTime + " -"
+ timeLabelEnd.text = endTime

I don't see why we cannot join them into one label by doing the following,

timeLabel = "%1 - \n %2".arg(startTime).arg(endTime)

Can you check if the above joint string works? That will also us to remove one Label{} from the ListItem.

4. In the design spec, the date label is shown as "October 28th, 2015". However in this branch they appear as "28 October, 2015". Can we fix this?

5. Personally I am not a big fan of the setDetails() function. I think it is a rather bad way of setting the listitem delegate values. However you did not introduce them in this MP. I will try changing it once this branch lands in trunk.

Can you fix issues #1-#4? 
-- 
https://code.launchpad.net/~majster-pl/ubuntu-calendar-app/new-agenda-view/+merge/287448
Your team Ubuntu Calendar Developers is subscribed to branch lp:ubuntu-calendar-app.