ubuntu-touch-coreapps-reviewers team mailing list archive
-
ubuntu-touch-coreapps-reviewers team
-
Mailing list archive
-
Message #08752
Re: [Merge] lp:~majster-pl/ubuntu-calendar-app/new-event-page into lp:ubuntu-calendar-app
Review: Needs Fixing
I'm setting this MP as a work in progress since it is. I have added inline comments of all things that needs fixing. This is just a preliminary check since I suspect more changes.
Do note that when I click the add guest button, I get ,
file:///opt/click.ubuntu.com/com.ubuntu.calendar/0.4.latest/NewEvent.qml:726:17: QML ScriptAction: file:///opt/click.ubuntu.com/com.ubuntu.calendar/0.4.latest/NewEvent.qml:728: ReferenceError: addGuestButton is not defined
if (addGuestButton.contactsPopup) {
Not sure if this was introduced in your branch or in trunk by some other branch. Please do check to make sure you don't introduce a regression.
I also see file:///opt/click.ubuntu.com/com.ubuntu.calendar/0.4.latest/NewEvent.qml:347:28: Unable to assign [undefined] to QColor
Diff comments:
> === modified file 'NewEvent.qml'
> --- NewEvent.qml 2016-03-07 17:57:04 +0000
> +++ NewEvent.qml 2016-03-10 13:19:38 +0000
> @@ -333,43 +337,51 @@
>
> header: PageHeader {
> id: pageHeader
> -
> - flickable: null
> - title: isEdit ? i18n.tr("Edit Event"):i18n.tr("New Event")
> - leadingActionBar.actions: Action {
> - id: backAction
> -
> - name: "cancel"
> - text: i18n.tr("Cancel")
> - iconName: isEdit ? "back" : "down"
> - onTriggered: root.cancel()
> - }
> -
> - trailingActionBar.actions: [
> - Action {
> - text: i18n.tr("Delete");
> - objectName: "delete"
> - iconName: "delete"
> - visible : isEdit
> - onTriggered: {
> - var dialog = PopupUtils.open(Qt.resolvedUrl("DeleteConfirmationDialog.qml"),root,{"event": event});
> - dialog.deleteEvent.connect( function(eventId){
> - model.removeItem(eventId);
> - if (pageStack)
> - pageStack.pop();
> - root.eventDeleted(eventId);
> - });
> - }
> - },
> - Action {
> - iconName: "ok"
> - objectName: "save"
> + property Component delegate: Component {
> + AbstractButton {
> + id: button
> + action: modelData
> + width: label.width + units.gu(4)
> + height: parent.height
> + Rectangle {
> + color: parent.pressed ? UbuntuColors.coolGrey : UbuntuColors.slate
> + opacity: 0.1
> + anchors.fill: parent
> + visible: button.pressed
> + }
> + Label {
> + anchors.centerIn: parent
> + id: label
> + text: action.text
> + font.weight: text === i18n.tr("Save") ? Font.Normal : Font.Light
> + }
> + }
> + }
> + leadingActionBar {
> + anchors.leftMargin: 0
> + actions: Action {
It is appropriate to add keyboard shortcut: "Esc" or Qt.Esc Key (can't remember of the top of my head, please do look it up) to activate the "Cancel" action.
> + text: i18n.tr("Cancel")
> + iconName: "close"
> + onTriggered: root.cancel()
> + }
> + delegate: pageHeader.delegate
> + }
> + trailingActionBar {
> + anchors.rightMargin: 0
> + actions: Action {
> text: i18n.tr("Save")
> - enabled: !!titleEdit.text.trim()
> + iconName: "tick"
> onTriggered: saveToQtPim();
Likewise here as well. Let's use Ctrl+S keyboard shortcut to save the event. If you look at https://developer.ubuntu.com/api/apps/qml/sdk-15.04.1/Ubuntu.Components.Action/ you will see there is a "Shortcut" property.
> }
> - ]
> - }
> + delegate: pageHeader.delegate
> + }
> + }
> +
> + Rectangle {
I see no need for this rectangle considering that the page background is already white (we already set it mainview).
> + anchors.fill: parent
> + color: "#FFFFFF"
> + }
> +
>
> Component{
> id: errorDlgComponent
> @@ -437,7 +453,8 @@
>
> NewEventTimePicker{
> id: startDateTimeInput
> - header: i18n.tr("From")
> + //TRANSLATORS: this referes to date. eg: To: Wendsday, 9 March 2016
Small comment error. In the eg, change "To" to "From" to make it more appropriate. (nitpick, sry ;)
> + headerText: i18n.tr("From")
> showTimePicker: !allDayEventCheckbox.checked
> anchors {
> left: parent.left
> @@ -461,232 +479,187 @@
> }
> }
>
> - ListItems.Standard {
> - anchors {
> - left: parent.left
> - right: parent.right
> - }
> -
> - text: i18n.tr("All day event")
> - showDivider: false
> - control: CheckBox {
> - objectName: "allDayEventCheckbox"
> - id: allDayEventCheckbox
> - checked: false
> - }
> - }
> -
> - ListItems.ThinDivider {}
> -
> - Column {
> - width: parent.width
> - spacing: units.gu(1)
> -
> - ListItems.Header{
> - text: i18n.tr("Event Details")
> - }
> -
> - TextField {
> - id: titleEdit
> - objectName: "newEventName"
> -
> - anchors {
> - left: parent.left
> - right: parent.right
> - margins: units.gu(2)
> - }
> -
> - inputMethodHints: Qt.ImhNoPredictiveText
> - placeholderText: i18n.tr("Event Name")
> - onFocusChanged: {
> - if(titleEdit.focus) {
> - flickable.makeMeVisible(titleEdit);
> - }
> - }
> - }
> -
> - TextArea{
> - id: messageEdit
> - objectName: "eventDescriptionInput"
> -
> - anchors {
> - left: parent.left
> - right: parent.right
> - margins: units.gu(2)
> - }
> -
> - placeholderText: i18n.tr("Description")
> - onFocusChanged: {
> - if(messageEdit.focus) {
> - flickable.makeMeVisible(messageEdit);
> - }
> - }
> - }
> -
> - TextField {
> - id: locationEdit
> - objectName: "eventLocationInput"
> -
> - anchors {
> - left: parent.left
> - right: parent.right
> - margins: units.gu(2)
> - }
> -
> - inputMethodHints: Qt.ImhNoPredictiveText
> - placeholderText: i18n.tr("Location")
> -
> - onFocusChanged: {
> - if(locationEdit.focus) {
> - flickable.makeMeVisible(locationEdit);
> - }
> - }
> - }
> - }
> -
> - Column {
> - width: parent.width
> - spacing: units.gu(1)
> -
> - ListItems.Header {
> - text: i18n.tr("Calendar")
> - }
> -
> - OptionSelector{
> - id: calendarsOption
> - objectName: "calendarsOption"
> -
> - anchors {
> - left: parent.left
> - right: parent.right
> - margins: units.gu(2)
> - }
> -
> - containerHeight: itemHeight * 4
> - model: root.model.getWritableCollections();
> -
> - delegate: OptionSelectorDelegate{
> - text: modelData.name
> -
> - UbuntuShape{
> - id: calColor
> - width: height
> - height: parent.height - units.gu(2)
> - color: modelData.color
> - anchors {
> - right: parent.right
> - rightMargin: units.gu(4)
> - verticalCenter: parent.verticalCenter
> - }
> - }
> - }
> - onExpandedChanged: Qt.inputMethod.hide();
> - }
> - }
> -
> - Column {
> - width: parent.width
> - spacing: units.gu(1)
> -
> - ListItems.Header {
> - text: i18n.tr("Guests")
> - }
> -
> - Button{
> - id: addGuestButton
> - objectName: "addGuestButton"
> -
> - property var contactsPopup: null
> -
> - text: i18n.tr("Add Guest")
> - anchors {
> - left: parent.left
> - right: parent.right
> - margins: units.gu(2)
> - }
> -
> - onClicked: {
> - if (contactsPopup)
> - return
> -
> - flickable.makeMeVisible(addGuestButton)
> - contactsPopup = PopupUtils.open(Qt.resolvedUrl("ContactChoicePopup.qml"), addGuestButton);
> - contactsPopup.contactSelected.connect( function(contact, emailAddress) {
> - if(!internal.isContactAlreadyAdded(contact, emailAddress) ) {
> - var t = internal.contactToAttendee(contact, emailAddress);
> - contactModel.append({"contact": t});
> - }
> -
> - });
> - contactsPopup.Component.onDestruction.connect( function() {
> - addGuestButton.contactsPopup = null
> - })
> - }
> - }
> -
> - UbuntuShape {
> - anchors {
> - left: parent.left
> - right: parent.right
> - margins: units.gu(2)
> - }
> -
> - height: contactList.height
> -
> - Column{
> - id: contactList
> - objectName: "guestList"
> -
> - spacing: units.gu(1)
> - width: parent.width
> - clip: true
> -
> - ListModel{
> - id: contactModel
> - }
> -
> - Repeater{
> - model: contactModel
> - delegate: ListItem {
> - objectName: "eventGuest%1".arg(index)
> -
> - ListItemLayout {
> - title.text: contact.name
> - subtitle.text: contact.emailAddress
> - }
> -
> - leadingActions: ListItemActions {
> - actions: Action {
> - iconName: "delete"
> - onTriggered: {
> - contactModel.remove(index)
> - }
> - }
> - }
> - }
> - }
> - }
> - }
> -
> - ListItems.ThinDivider {
> - visible: (event != undefined) && (event.itemType === Type.Event)
> - }
> -
> - }
> -
> - ListItems.Subtitled{
> + // All day event ListItem with Switch
> + ListItem {
> + width: parent.width
ListItem default to width: parent.width. We don't need to explicitly mention it unless we set some left/right anchor margins.
> +
> + ListItemLayout {
> + title.text: i18n.tr("All day event")
> + Switch {
> + id: allDayEventCheckbox
> + checked: false
> + SlotsLayout.position: SlotsLayout.Trailing;
> + }
> + }
> + onClicked: {
> + Haptics.play()
> + allDayEventCheckbox.checked = !allDayEventCheckbox.checked
> + }
> +
> + }
> +
> + // ListItem which holds "Event details" label + TextField + TextArea + TextField
> + ListItem {
> + width: parent.width
Same here. remove the width: parent.width
> + height: eventDetailsColumn.height + (eventDetailsColumn.anchors.margins*2)
> +
> + Column {
> + id: eventDetailsColumn
> + spacing: units.gu(2)
> + anchors {
> + left: parent.left
> + right: parent.right
> + top: parent.top
> + margins: units.gu(2)
> + }
> +
> + Label {
> + width: parent.width
> + text: i18n.tr("Event details")
> + elide: Text.ElideRight
> + }
> +
> + TextField {
> + id: titleEdit
> + objectName: "newEventName"
> +
> + width: parent.width
> + inputMethodHints: Qt.ImhNoPredictiveText
> + placeholderText: i18n.tr("Event Name")
> +
> + onFocusChanged: {
> + if(titleEdit.focus) {
> + flickable.makeMeVisible(titleEdit);
> + }
> + }
> + }
> +
> + TextArea{
> + id: messageEdit
> + objectName: "eventDescriptionInput"
> +
> + width: parent.width
> + placeholderText: i18n.tr("Description")
> +
> + onFocusChanged: {
> + if(messageEdit.focus) {
> + flickable.makeMeVisible(messageEdit);
> + }
> + }
> + }
> +
> + TextField {
> + id: locationEdit
> + objectName: "eventLocationInput"
> +
> + width: parent.width
> + inputMethodHints: Qt.ImhNoPredictiveText
> + placeholderText: i18n.tr("Location")
> +
> + onFocusChanged: {
> + if(locationEdit.focus) {
> + flickable.makeMeVisible(locationEdit);
> + }
> + }
> + }
> + }
> + }
> +
> + // ListItem to hold calendars selector
> + ListItem {
> + width: parent.width
> + height: chooseCalendarColumn.height + (eventDetailsColumn.anchors.topMargin*2)
> +
> + Column {
> + id: chooseCalendarColumn
> + spacing: units.gu(2)
> + anchors {
> + left: parent.left
> + right: parent.right
> + top: parent.top
> + topMargin: units.gu(2)
> + }
> +
> + Label {
> + width: parent.width
> + anchors {
> + left: parent.left
> + leftMargin: units.gu(2)
> + right: parent.right
> + rightMargin: units.gu(2)
> + }
> + text: i18n.tr("Choose calendar")
> + elide: Text.ElideRight
> + }
> +
> + ListItems.ItemSelector {
> + id: calendarsOption
> + model: root.model.getWritableCollections();
> + delegate: OptionSelectorDelegate { text: modelData.name }
> + }
> +
> + }
> + }
> +
> +
> + ListItem {
> + width: parent.width
> + height: guestsColumn.height + (guestsColumn.anchors.margins*2)
> +
> + Column {
> + id: guestsColumn
> + spacing: units.gu(2)
> + anchors {
> + left: parent.left
> + right: parent.right
> + top: parent.top
> + margins: units.gu(2)
> + }
> +
> + Label {
> + width: parent.width
> + text: i18n.tr("Guests")
> + elide: Text.ElideRight
> + }
> +
> + TextField {
> + id: guestsEdit
> + objectName: "eventLocationInput"
> +
> + width: parent.width
> + inputMethodHints: Qt.ImhNoPredictiveText
> + placeholderText: i18n.tr("Add guests")
> +
> + onFocusChanged: {
> + if(guestsEdit.focus) {
> + flickable.makeMeVisible(guestsEdit);
> + }
> + }
> + }
> +
> +
> +
> + }
> + }
> +
> +
> + ListItem {
> id:thisHappens
> objectName :"thisHappens"
>
> - anchors {
> - left: parent.left
> + width: parent.width
No need for width: parent.width
> + height: thisHappensLayout.height
> +
> + ListItemLayout {
> + id: thisHappensLayout
> + title.text: i18n.tr("Repeats")
> + summary.text: (event != undefined) && (event.itemType === Type.Event) ? rule === null ? i18n.tr(Defines.recurrenceLabel[0]) : eventUtils.getRecurrenceString(rule) : ""
Here the code formatting is a bit difficult to read. Please change it to,
summary.text: (event != undefined) && (event.itemType === Type.Event) ? rule === null ? i18n.tr(Defines.recurrenceLabel[0])
: eventUtils.getRecurrenceString(rule)
: ""
> + Icon {
This whole icon can be replaced by ProgressionSlot{}. Please take a look at https://developer.ubuntu.com/api/apps/qml/sdk-15.04.1/Ubuntu.Components.ProgressionSlot/
> + name: "go-next"
> + SlotsLayout.position: SlotsLayout.Trailing;
> + width: units.gu(2.5)
> + }
> }
> -
> - showDivider: false
> - progression: true
> - visible: (event != undefined) && (event.itemType === Type.Event)
> - text: i18n.tr("Repeats")
> - subText: (event != undefined) && (event.itemType === Type.Event) ? rule === null ? Defines.recurrenceLabel[0] : eventUtils.getRecurrenceString(rule) : ""
> onClicked: {
> var stack = pageStack
> if (!stack)
> @@ -696,41 +669,28 @@
> }
> }
>
> - ListItems.ThinDivider {
> - visible: (event != undefined) && (event.itemType === Type.Event)
> - }
>
> - ListItems.Subtitled{
> + ListItem {
> id:eventReminder
> objectName : "eventReminder"
>
> - anchors.left:parent.left
> - showDivider: false
> - progression: true
> - text: i18n.tr("Reminder")
> -
> - RemindersModel {
> - id: reminderModel
> - }
> -
> - subText:{
> - if(visualReminder.secondsBeforeStart !== -1) {
> - for( var i=0; i<reminderModel.count; i++ ) {
> - if(visualReminder.secondsBeforeStart === reminderModel.get(i).value) {
> - return reminderModel.get(i).label
> - }
> - }
> - } else {
> - return reminderModel.get(0).label
> + width: parent.width
No need for width: parent.width
> + height: eventReminderLayout.height
> +
> + ListItemLayout {
> + id: eventReminderLayout
> + title.text: i18n.tr("Reminder")
> + subtitle.text: i18n.tr("%1").arg(reminderModel.getSelectedReminder())
> + Icon {
This whole icon can be replaced by ProgressionSlot{}. Please take a look at https://developer.ubuntu.com/api/apps/qml/sdk-15.04.1/Ubuntu.Components.ProgressionSlot/
> + name: "go-next"
> + SlotsLayout.position: SlotsLayout.Trailing;
> + width: units.gu(2.5)
> }
> -
> }
>
> - onClicked:{
> + onClicked: {
> var stack = pageStack
> - if (!stack)
> - stack = bottomEdgePageStack
> -
> + if (!stack) stack = bottomEdgePageStack
> stack.push(Qt.resolvedUrl("EventReminder.qml"),
> {"visualReminder": visualReminder,
> "audibleReminder": audibleReminder,
>
> === modified file 'NewEventTimePicker.qml'
> --- NewEventTimePicker.qml 2016-01-25 17:52:33 +0000
> +++ NewEventTimePicker.qml 2016-03-10 13:19:38 +0000
> @@ -1,75 +1,113 @@
> import QtQuick 2.4
> -import Ubuntu.Components.ListItems 1.0 as ListItem
> +import Ubuntu.Components 1.3
> import Ubuntu.Components.Themes.Ambiance 1.0
> -import Ubuntu.Components.Pickers 1.0
> +import Ubuntu.Components.Pickers 1.3
> +//import QtQuick.Layouts 1.1
>
> -Column {
> +ListItem {
> id: dateTimeInput
> - property alias header: listHeader.text
> + property string headerText //header label ("From" or "To")
> + property date dateTime //keep date from DatePicker
> + property bool showTimePicker //if true then user is able to set time on event
>
> - property date dateTime;
> - property bool showTimePicker;
> + // when new date set in DatePicker then this will be run.
> + onDateTimeChanged: {
> + layout.summary.text = dateTime.toLocaleDateString() // set date
> + secondLabel.text = Qt.formatTime(dateTime, "hh:mm AP").replace(/\./g, "") // set time
> + }
>
> function clearFocus() {
> - dateInput.focus = false;
> - timeInput.focus = false;
> + dateBG.focus = false;
> + timeBG.focus = false;
> }
>
> + // function to open date/time picker
> function openDatePicker (element, caller, callerProperty, mode) {
> element.highlighted = true;
> var picker = PickerPanel.openDatePicker(caller, callerProperty, mode);
> if (!picker) return;
> - picker.closed.connect(function () {
> - element.highlighted = false;
> - });
> - }
> -
> - onDateTimeChanged: {
> - dateInput.text = dateTime.toLocaleDateString();
> - timeInput.text = Qt.formatTime(dateTime);
> - }
> -
> - ListItem.Header {
> - id: listHeader
> - }
> -
> - Item {
> - anchors {
> - left: parent.left
> - right: parent.right
> - margins: units.gu(2)
> - }
> -
> - height: dateInput.height
> -
> - NewEventEntryField{
> - id: dateInput
> - objectName: "dateInput"
> -
> - text: ""
> - anchors.left: parent.left
> - width: !showTimePicker ? parent.width : 4 * parent.width / 5
> -
> - MouseArea{
> - anchors.fill: parent
> - onClicked: openDatePicker(dateInput, dateTimeInput, "dateTime", "Years|Months|Days")
> - }
> - }
> -
> - NewEventEntryField{
> - id: timeInput
> - objectName: "timeInput"
> -
> - text: ""
> - anchors.right: parent.right
> - width: parent.width / 5
> + picker.closed.connect(function () { element.highlighted = false; });
> + }
> +
> + height: layout.height + divider.height
> +// divider.height: units.gu(0.1)
> +
> +
> + // backgroud color of full date fabel, to be shown when user click on date and DatePicker is visable
> + Rectangle {
> + id: dateBG
> +
> + property bool highlighted: false
> +
> + height: layout.summary.height + units.gu(3.5)
> + width: showTimePicker ? layout.title.width + units.gu(3) : layout.width
> + anchors.bottom: parent.bottom
> + color: highlighted ? UbuntuColors.coolGrey : "transparent"
> +
> + Behavior on color { ColorAnimation {} }
> + }
> +
> + // backgroud color of time label, to be shown when user click on date and DatePicker is visable
> + Rectangle {
> + id: timeBG
> +
> + property bool highlighted: false
> +
> + height: dateBG.height
> + width: slot.width + units.gu(4)
> + anchors.bottom: parent.bottom
> + anchors.right: parent.right
> + color: highlighted ? UbuntuColors.coolGrey : "transparent"
> +
> + Behavior on color { ColorAnimation {} }
> + }
> +
> + // ListItemLayout to keep full date label and time label
> + ListItemLayout {
> + id: layout
> +
> + title.text: headerText
> + title.color: UbuntuColors.Orange
> + title.font.pixelSize: FontUtils.sizeToPixels("small")
> + subtitle.text: " "
> + summary.color: dateBG.highlighted ? UbuntuColors.orange : Theme.palette.selected.fieldText
> + summary.font.pixelSize: FontUtils.sizeToPixels("medium")
> +
> + Behavior on summary.color { ColorAnimation {} }
> +
> +
> + // Item to hold Trailing tile label item
> + Item {
> + id: slot
> +
> + width: secondLabel.width
> + height: parent.height
> visible: showTimePicker
> - horizontalAlignment: Text.AlignRight
> -
> - MouseArea{
> - anchors.fill: parent
> - onClicked: openDatePicker(timeInput, dateTimeInput, "dateTime", "Hours|Minutes")
> + SlotsLayout.overrideVerticalPositioning: true
> +
> + // label to keep time [ 10:20 AM ]
> + Label {
> + id: secondLabel
> +
> + fontSize: "medium"
> + color: timeBG.highlighted ? UbuntuColors.orange : Theme.palette.selected.fieldText
> + y: layout.mainSlot.y + layout.summary.y + layout.summary.baselineOffset - baselineOffset
> + Behavior on color { ColorAnimation {} }
> }
> }
> }
> +
> + // MouseArea to be triggered when user click on full date
> + MouseArea {
I would suggest replacing MouseArea{} with AbstractButton since that will already bring in the necessary haptics when you click them. Thereby removing the need for Haptics.play()
> + anchors.fill: dateBG
> + onClicked: { openDatePicker(dateBG, dateTimeInput, "dateTime", "Years|Months|Days"); Haptics.play()}
> + }
> +
> + // MouseArea to be triggered when user click on time
> + MouseArea {
I would suggest replacing MouseArea{} with AbstractButton since that will already bring in the necessary haptics when you click them. Thereby removing the need for Haptics.play()
> + anchors.fill: timeBG
> + visible: showTimePicker
> + onClicked: { openDatePicker(timeBG, dateTimeInput, "dateTime", "Hours|Minutes"); Haptics.play()}
> + }
> +
> }
>
> === modified file 'PageWithBottomEdge.qml'
> --- PageWithBottomEdge.qml 2016-03-07 17:57:04 +0000
> +++ PageWithBottomEdge.qml 2016-03-10 13:19:38 +0000
> @@ -28,6 +28,7 @@
> signal bottomEdgeCommitStarted()
> signal eventCreated(var event)
>
> +
Please remove this change as it has nothing to do with your MP.
> function bottomEdgeCommit(date, allDay)
> {
> bottomEdge.commit()
>
> === modified file 'calendar.qml'
> --- calendar.qml 2016-03-07 15:58:43 +0000
> +++ calendar.qml 2016-03-10 13:19:38 +0000
> @@ -89,7 +89,7 @@
> objectName: "calendar"
> applicationName: "com.ubuntu.calendar"
>
> - width: units.gu(100)
> + width: units.gu(50)
Again, remove this change as the appropriate size of the calendar app on the desktop is actually 100 gu.
> height: units.gu(80)
> focus: true
> Keys.forwardTo: [pageStack.currentPage]
--
https://code.launchpad.net/~majster-pl/ubuntu-calendar-app/new-event-page/+merge/288637
Your team Ubuntu Calendar Developers is subscribed to branch lp:ubuntu-calendar-app.
