ubuntu-touch-coreapps-reviewers team mailing list archive

[Renato Araujo Oliveira Filho <renato.filho@xxxxxxxxxxxxx>]

Chekc my inline comments. I have some suggestion how to implement it in a better way.

Diff comments:

> === modified file 'EventActions.qml'
> --- EventActions.qml	2016-03-04 16:10:56 +0000
> +++ EventActions.qml	2016-03-14 19:04:24 +0000
> @@ -26,8 +26,16 @@
>      property alias showCalendarAction: _showCalendarAction
>      property alias syncCalendarAction: _syncCalendarAction
>      property alias settingsAction: _settingsAction
> +    property alias displayLunarCalendar: settingsPage.displayLunarCalendar
> +    property alias displayWeekNumber: settingsPage.displayWeekNumber
> +    property alias reminderDefaultValue: settingsPage.reminderDefaultValue

Why do you need replicate the properties here?

>      readonly property bool syncInProgress: (syncMonitor.state === "syncing")
>  
> +    Settings {

Why do you need the Settings pages object here? It will cause the page to be created on app startup.

> +        id: settingsPage
> +        onBackRequested: pageStack.pop()
> +    }
> +
>      Action {
>          id: _syncCalendarAction
>          objectName: "syncbutton"
> @@ -62,8 +70,6 @@
>          name: "calendarsbutton"
>          iconName: "settings"
>          text: i18n.tr("Settings")
> -        onTriggered: {
> -            pageStack.push(Qt.resolvedUrl("Settings.qml"));
> -        }
> +        onTriggered: pageStack.push(settingsPage);

Better create the page on demand. Why not?
Create a Settings object inside of Settings page and the values will be propagated to the main window.

>      }
>  }
> 
> === modified file 'NewEvent.qml'
> --- NewEvent.qml	2016-03-07 17:57:04 +0000
> +++ NewEvent.qml	2016-03-14 19:04:24 +0000
> @@ -42,6 +42,8 @@
>  
>      property var startDate;
>      property var endDate;
> +    //default reminder time = 15 min
> +    property int reminderValue;

should it be set to 900 as default? (15 min)

>  
>      property alias scrollY: flickable.contentY
>      property bool isEdit: false
> 
> === modified file 'calendar.qml'
> --- calendar.qml	2016-03-07 15:58:43 +0000
> +++ calendar.qml	2016-03-14 19:04:24 +0000
> @@ -259,12 +260,22 @@
>  
>          EventActions {
>              id: commonHeaderActions
> +
> +            displayWeekNumber: mainView.displayWeekNumber
> +            displayLunarCalendar: mainView.displayLunarCalendar
> +            reminderDefaultValue: mainView.reminderDefaultValue
> +
> +            onDisplayWeekNumberChanged: mainView.displayWeekNumber = displayWeekNumber
> +            onDisplayLunarCalendarChanged: mainView.displayLunarCalendar = displayLunarCalendar
> +            onReminderDefaultValueChanged: mainView.reminderDefaultValue = reminderDefaultValue 

Why you need this? The 'settings' object should propagate the properties change as soon it get saved on settings page.

>          }
>  
>          Settings {

This is very confuse :D. I know this was not created by you. But could you change the Settings page object name to SettingsPage to avoid confusion with Settings object.

>              id: settings
>              property alias defaultViewIndex: tabs.selectedTabIndex
>              property alias showWeekNumber: mainView.displayWeekNumber
> +            property alias showLunarCalendar: mainView.displayLunarCalendar
> +            property alias reminderDefaultValue: mainView.reminderDefaultValue
>          }
>  
>          Tabs{


-- 
https://code.launchpad.net/~artmello/ubuntu-calendar-app/ubuntu-calendar-app-fix_1554610/+merge/288969
Your team Ubuntu Calendar Developers is requested to review the proposed merge of lp:~artmello/ubuntu-calendar-app/ubuntu-calendar-app-fix_1554610 into lp:ubuntu-calendar-app.