ubuntu-touch-coreapps-reviewers team mailing list archive
-
ubuntu-touch-coreapps-reviewers team
-
Mailing list archive
-
Message #08389
Re: [Merge] lp:~majster-pl/ubuntu-calendar-app/new-agenda-view into lp:ubuntu-calendar-app
Review: Needs Fixing code-review
Added some inline comments that need fixing. Otherwise looks good.
Diff comments:
> === modified file 'AgendaView.qml'
> --- AgendaView.qml 2016-02-03 08:06:25 +0000
> +++ AgendaView.qml 2016-02-29 10:27:23 +0000
> @@ -19,7 +19,7 @@
> import QtQuick 2.4
> import QtOrganizer 5.0
> import Ubuntu.Components 1.3
> -import Ubuntu.Components.ListItems 1.0 as ListItem
> +//import Ubuntu.Components.ListItems 1.0 as ListItem
Ubuntu.Components.ListItems 1.0 has been deprecated and I doubt we would use it again. Let's remove this comment as well.
> import "dateExt.js" as DateExt
> import "./3rd-party/lunar.js" as Lunar
>
> @@ -174,7 +161,8 @@
>
> // TRANSLATORS: the first argument (%1) refers to a start time for an event,
> // while the second one (%2) refers to the end time
> - var timeString = i18n.tr("%1 - %2").arg(startTime).arg(endTime)
> + var timeStringStart = i18n.tr("%1").arg(startTime) + " -"
> + var timeStringEnd = i18n.tr("%1").arg(endTime)
Since agenda view consists of a listview with potentially several listitems, it is best to optimize it where we can. I suggest we stick to using one string to reduce listitem creation time.
var timeString = i18n.tr("%1 -\n %2").arg(startTime).arg(endTime)
Add "\n" should move the after that character to the next line. Could you try that? This way we remove one string correction.
>
> if (mainView.displayLunarCalendar) {
> var lunarDate = Lunar.calendar.solar2lunar(event.startDateTime.getFullYear(),
> @@ -216,65 +221,119 @@
> elide: Text.ElideRight
> anchors {
> left: parent.left
> + leftMargin: units.gu(2)
> + verticalCenter: parent.verticalCenter
> + }
> + }
> + // MouseArea, onClicked new page with dayle view will open.
> + MouseArea {
> + id: headerMouseArea
> + anchors.fill: parent
> + onClicked: {
> + Haptics.play()
> + dateSelected(event.startDateTime);
> + }
> + }
> +
> + }
> +
> + // Main ListItem to hold Details about event eg. ( 19:30 - Beer with the team )
> + // ( 20:00 Hicter )
> + ListItem {
> + id: detailsListItem
> + width: parent.width
> + height: detailsColumn.height + units.gu(4)
> +
> + // ListItem background (changing color when pressed)
> + Rectangle {
> + anchors.fill: parent
> + color: detailsMouseArea.pressed ? "#f7f7f7" : "white"
> + }
> +
> + // Little icon in left top corner of every event to indicate color of the calendar.
> + UbuntuShape {
> + id: calendarIndicator
> + anchors {
> + left: parent.left;
> leftMargin: units.gu(1)
> + top: parent.top;
> + topMargin: units.gu(1)
> + }
> + width: units.gu(1)
> + height: width
> + aspect: UbuntuShape.DropShadow
> + }
> +
> + // main Column to hold Grid with times, Description and location.
> + Column{
Here instead of using a column and setting labels and their positioning manually, I suggest we use ListItemLayouts [1]. This would essentially simplify the code to,
ListItem {
height: listitemlayout.height + divider.height
ListItemLayout {
id: listitemlayout
title.text: i18n.tr("%1").arg(event.displayLabel)
title.font.bold: true
subtitle.text: i18n.tr("%1").arg(event.location)
subtitle.color: UbuntuColors.coolGrey
Label{
objectName: "timeLabel" + index
id: timeLabelStart
color:"White"
objectName: "timeLabelStart" + index
color: UbuntuColors.coolGrey
fontSize: "small"
SlotsLayouts.position: SlotsLayout.leading
}
}
}
ListItemLayout will automatically take care of the left and right margin, eliding and so on. Check it out :)
[1] https://developer.ubuntu.com/api/apps/qml/sdk-15.04.1/Ubuntu.Components.ListItemLayout/
> + id: detailsColumn
> + anchors {
> + left: parent.left
> + right: parent.right
> verticalCenter: parent.verticalCenter
> }
> - }
> - onClicked: {
> - dateSelected(event.startDateTime);
> - }
> - showDivider: false
> - }
> - ListItem.ThinDivider {}
> -
> - Rectangle{
> - id: detailsContainer
> -
> - anchors {
> - left: parent.left
> - right: parent.right
> - }
> -
> - height: detailsColumn.height + units.gu(1)
> -
> - ListItem.Standard{
> - showDivider:false
> - Column{
> - id: detailsColumn
> -
> +
> + // Mian Grid
> + Grid {
> + columns: 2
> + columnSpacing: units.gu(1.5)
> anchors {
> - top: parent.top
> left: parent.left
> + leftMargin: units.gu(3)
> right: parent.right
> - margins: units.gu(0.5)
> }
>
> - Label{
> - id: timeLabel
> - objectName: "timeLabel" + index
> - color:"White"
> + // Event time start
> + Label{
> + id: timeLabelStart
> + objectName: "timeLabelStart" + index
> + color: UbuntuColors.coolGrey
> + fontSize: "small"
> + }
> + // Event descriptions
> + Label{
> + id: eventLabel
> + objectName: "eventLabel" + index
> + width: parent.width - timeLabelStart.width - parent.columnSpacing
> + elide: Text.ElideRight
> + color: UbuntuColors.coolGrey
> font.bold: true
> fontSize: "small"
> - width: parent.width
> }
>
> + // Event time End
> Label{
> - id: titleLabel
> - objectName: "titleLabel" + index
> -
> - color:"White"
> + id: timeLabelEnd
> + objectName: "timeLabelEnd" + index
> + color: UbuntuColors.coolGrey
> fontSize: "small"
> - width: parent.width
> - maximumLineCount: 2
> + }
> +
> + // Event Location
> + Label{
> + id: locationLabel
> + objectName: "locationLabel" + index
> + width: parent.width - timeLabelStart.width - parent.columnSpacing
> elide: Text.ElideRight
> - wrapMode: Text.WrapAtWordBoundaryOrAnywhere
> + color: UbuntuColors.coolGrey
> + fontSize: "small"
> }
> }
> +
> +
> + }
> +
> + // 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});
> }
> }
> }
> +
> }
> }
> }
--
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.
References
