ubuntu-touch-coreapps-reviewers team mailing list archive
-
ubuntu-touch-coreapps-reviewers team
-
Mailing list archive
-
Message #08207
Re: [Merge] lp:~ubuntu-clock-dev/ubuntu-clock-app/ubuntu-clock-app-new-design into lp:ubuntu-clock-app
Review: Needs Fixing
Generally it works awensome.
Some coding style and comment improvements needs to be done, before merging.
Diff comments:
>
> === modified file 'app/alarm/EditAlarmPage.qml'
> --- app/alarm/EditAlarmPage.qml 2015-12-11 02:11:11 +0000
> +++ app/alarm/EditAlarmPage.qml 2016-02-17 21:48:16 +0000
> @@ -266,39 +266,60 @@
> }
> }
>
> - SubtitledListItem {
> + ListItem {
> id: _alarmRepeat
> objectName: "alarmRepeat"
>
> - text: i18n.tr("Repeat")
> - subText: alarmUtils.format_day_string(_alarm.daysOfWeek, _alarm.type)
> + property string subText: alarmUtils.format_day_string(_alarm.daysOfWeek, _alarm.type)
> + height: alarmRepeatLayout.height + divider.height
> +
> + ListItemLayout {
> + id: alarmRepeatLayout
> +
> + title.text: i18n.tr("Repeat")
> + subtitle.text: _alarmRepeat.subText
> + }
> onClicked: pageStack.push(Qt.resolvedUrl("AlarmRepeat.qml"),
> {"alarm": _alarm, "alarmUtils": alarmUtils})
> }
>
> - SubtitledListItem {
> + ListItem {
> id: _alarmLabel
> objectName: "alarmLabel"
> -
> - text: i18n.tr("Label")
> - subText: _alarm.message
> + height: alarmLabelLayout.height + divider.height
> +
> + ListItemLayout {
> + id: alarmLabelLayout
> +
> + title.text: i18n.tr("Label")
> + subtitle.text: _alarm.message
> + }
> onClicked: pageStack.push(Qt.resolvedUrl("AlarmLabel.qml"),
> {"alarm": _alarm})
> }
>
> - SubtitledListItem {
> + ListItem {
> id: _alarmSound
> objectName: "alarmSound"
>
> readonly property string defaultAlarmSound: "Alarm clock"
> readonly property string defaultAlarmSoundFileName: "Alarm clock.ogg"
> -
> - text: i18n.tr("Sound")
> + property string subText
> +
> + height: alarmSoundLayout.height + divider.height
> +
> + ListItemLayout {
> + id: alarmSoundLayout
> +
> + title.text: i18n.tr("Sound")
> + subtitle.text: _alarmSound.subText
> + }
> onClicked: pageStack.push(Qt.resolvedUrl("AlarmSound.qml"), {
> "alarmSound": _alarmSound,
> "alarm": _alarm
> })
> }
> +
Please remove this new line
> }
>
> Button {
>
> === modified file 'app/clock/ClockPage.qml'
> --- app/clock/ClockPage.qml 2016-01-16 08:18:05 +0000
> +++ app/clock/ClockPage.qml 2016-02-17 21:48:16 +0000
> @@ -201,16 +201,14 @@
> id: date
>
> anchors {
> - top: parent.top
> - topMargin: units.gu(41)
> + top: clock.bottom
> + //topMargin: units.gu(29)
please remove commented code
> horizontalCenter: parent.horizontalCenter
> }
>
> text: clock.localizedDateString
> -
> + textSize: Label.Small
> opacity: 0
> - color: locationRow.visible ? Theme.palette.normal.baseText : UbuntuColors.midAubergine
> - fontSize: "medium"
> }
>
> Row {
>
> === modified file 'app/components/ClockCircle.qml'
> --- app/components/ClockCircle.qml 2016-01-16 08:35:09 +0000
> +++ app/components/ClockCircle.qml 2016-02-17 21:48:16 +0000
> @@ -30,31 +30,29 @@
>
> If used as the inner circle, a textured background image is set as part of
> the new design specs.
> -
> - The circle position is set by the public property "isOuter". If true, the
> - outer circle characteristics are set and vice-versa.
> */
Ten komentarz jest juz nieprawidlowy:
/*
Clock Circle with the shadows and background color set depending on the
position of the circle.
If used as the outer circle, the shadows are at the top and the background
has a 30% white shade. On the other hand, if used as the inner clock circle
then the shadows are at the bottom and the background has a 3% darker shade.
If used as the inner circle, a textured background image is set as part of
the new design specs.
*/
> Circle {
> id: _innerCircle
>
> - /*
> - Property to set if the circle is the outer or the inner circle
> - */
> - property bool isOuter: false
> + property bool isFoldVisible: true
>
> - color: isOuter ? "#4DFFFFFF" : "#08000000"
> - borderWidth: units.gu(0.2)
> - borderColorTop: isOuter ? "#6E6E6E" : "#00000000"
> - borderColorBottom: isOuter ? "#00000000" : "#6E6E6E"
> + color: "#f7f7f7"
> + borderWidth: units.dp(1)/*units.gu(0.2)*/
Please remove commented code
> + borderColorTop: "#00000000"
> + borderColorBottom: "#6E6E6E"
> borderOpacity: 0.65
> - borderGradientPosition: isOuter ? 0.7 : 0.2
> + borderGradientPosition: 0.2
>
> - Image {
> + Rectangle {
> + visible: isFoldVisible
> anchors.fill: parent
> - anchors.margins: borderWidth / 2.0
> - smooth: true
> - fillMode: Image.PreserveAspectFit
> - source: !isOuter ? "../graphics/Inner_Clock_Texture.png" : ""
> - cache: false
> + anchors.margins: borderWidth
> + radius: height/2
spaces around operator
> + gradient: Gradient {
> + GradientStop { position: 0.0; color: "#f7f7f7" }
> + GradientStop { position: 0.5; color: "#f7f7f7" }
> + GradientStop { position: 0.51; color: "#fdfdfd" }
> + GradientStop { position: 1.0; color: "#fdfdfd" }
> + }
> }
> }
>
> === modified file 'app/components/HeaderNavigation.qml'
> --- app/components/HeaderNavigation.qml 2015-10-22 16:49:23 +0000
> +++ app/components/HeaderNavigation.qml 2016-02-17 21:48:16 +0000
> @@ -22,51 +22,34 @@
> Item {
> id: headerRow
>
> - height: units.gu(7)
> + height: units.gu(6)
>
> Row {
> id: iconContainer
>
> anchors.centerIn: parent
> + spacing: units.gu(2)
>
> ActionIcon {
> - width: units.gu(5.5)
> - height: units.gu(4)
> - iconWidth: units.gu(4)
> - iconSource: Qt.resolvedUrl("../graphics/WorldClock_Placeholder.svg")
> + iconName: "clock"
> + iconColor: listview.currentIndex == 0 ? "#19b6ee" : "#5d5d5d"
> onClicked: listview.currentIndex = 0
> }
>
> ActionIcon {
> - width: units.gu(5.5)
> - height: units.gu(4)
> - iconWidth: units.gu(4)
> - iconSource: Qt.resolvedUrl("../graphics/Stopwatch_Placeholder.svg")
> + iconName: "stopwatch"
> + iconColor: listview.currentIndex == 1 ? "#19b6ee" : "#5d5d5d"
> onClicked: listview.currentIndex = 1
> }
> - }
> -
> - Rectangle {
> - id: outerProgressRectangle
> - anchors {
> - left: iconContainer.left
> - right: iconContainer.right
> - top: iconContainer.bottom
> - }
> - height: units.gu(0.3)
> - color: UbuntuColors.lightGrey
> -
> - Rectangle {
> - height: parent.height
> - x: listview.currentIndex == 0 ? 0 : parent.width - width
> - width: units.gu(5.5)
> - color: UbuntuColors.orange
> - Behavior on x {
> - UbuntuNumberAnimation {}
> - }
> - }
> - }
> -
> +
> +// ActionIcon {
> +// iconName: "timer"
> +// iconColor: listview.currentIndex == 2 ? "#19b6ee" : "#5d5d5d"
> +// onClicked: listview.currentIndex = 2
> +// }
please remove commented code
> + }
> +
> +
> ActionIcon {
> id: settingsIcon
> objectName: "settingsIcon"
>
> === modified file 'app/stopwatch/LapListView.qml'
> --- app/stopwatch/LapListView.qml 2015-10-22 16:49:23 +0000
> +++ app/stopwatch/LapListView.qml 2016-02-17 21:48:16 +0000
> @@ -32,38 +32,38 @@
>
> header: ListItem {
> visible: count !== 0
> + width: parent.width - units.gu(4)
> + anchors.horizontalCenter: parent.horizontalCenter
> +
> Row {
> anchors {
> left: parent.left
> right: parent.right
> verticalCenter: parent.verticalCenter
> - margins: units.gu(2)
> + margins: units.gu(1)
> }
>
> Label {
> // #TRANSLATORS: This refers to the stopwatch lap and is shown as a header where space is limited. Constrain
> // translation length to a few characters.
> text: i18n.tr("Lap")
> - width: parent.width / 7
> + width: parent.width /5
> elide: Text.ElideRight
> - font.weight: Font.DemiBold
> - horizontalAlignment: Text.AlignHCenter
> + horizontalAlignment: Text.AlignLeft
> }
>
> Label {
> - width: 3 * parent.width / 7
> + width: parent.width *2/5
spaces around operator
> elide: Text.ElideRight
> text: i18n.tr("Lap Time")
> - font.weight: Font.DemiBold
> horizontalAlignment: Text.AlignHCenter
> }
>
> Label {
> - width: 3 * parent.width / 7
> + width: parent.width *2/5
spaces around operator
> elide: Text.ElideRight
> text: i18n.tr("Total Time")
> - font.weight: Font.DemiBold
> - horizontalAlignment: Text.AlignHCenter
> + horizontalAlignment: Text.AlignRight
> }
> }
> }
> @@ -93,18 +96,17 @@
> left: parent.left
> right: parent.right
> verticalCenter: parent.verticalCenter
> - margins: units.gu(2)
> + leftMargin: units.gu(1)
> }
>
> Label {
> - color: UbuntuColors.midAubergine
> text: "#%1".arg(Number(count - index).toLocaleString(Qt.locale(), "f", 0))
> - width: parent.width / 7
> - horizontalAlignment: Text.AlignHCenter
> + width: parent.width/5
spaces around operator
> + horizontalAlignment: Text.AlignLeft
> }
>
> Item {
> - width: 3 * parent.width / 7
> + width: parent.width *2/5
spaces around operator
> height: childrenRect.height
> Row {
> anchors.horizontalCenter: parent.horizontalCenter
>
> === modified file 'app/stopwatch/StopwatchPage.qml'
> --- app/stopwatch/StopwatchPage.qml 2015-10-22 16:49:23 +0000
> +++ app/stopwatch/StopwatchPage.qml 2016-02-17 21:48:16 +0000
> @@ -61,7 +72,10 @@
>
> Button {
> id: stopButton
> - width: stopwatchEngine.previousTimeOfStopwatch !== 0 || stopwatchEngine.running ? (parent.width - parent.spacing) / 2 : parent.width
> +
> + width: parent.width/2 - units.gu(1)
spaces around operator
> + height: units.gu(4)
> + x: stopwatchEngine.previousTimeOfStopwatch !== 0 || stopwatchEngine.running ? 0 : (parent.width - width)/2
> color: !stopwatchEngine.running ? UbuntuColors.green : UbuntuColors.red
> text: stopwatchEngine.running ? i18n.tr("Stop") : (stopwatchEngine.previousTimeOfStopwatch === 0 ? i18n.tr("Start") : i18n.tr("Resume"))
> onClicked: {
--
https://code.launchpad.net/~ubuntu-clock-dev/ubuntu-clock-app/ubuntu-clock-app-new-design/+merge/281972
Your team Ubuntu Clock Developers is subscribed to branch lp:ubuntu-clock-app.
References
