ubuntu-touch-coreapps-reviewers team mailing list archive

[Riccardo Padovani <riccardo@xxxxxxxxxxxxx>]

Review: Needs Fixing

Great improvement!
Some feedbacks inline...

Diff comments:

> === modified file 'src/app/qml/components/EditTagsDialog.qml'
> --- src/app/qml/components/EditTagsDialog.qml	2015-02-27 22:43:12 +0000
> +++ src/app/qml/components/EditTagsDialog.qml	2015-06-11 19:39:25 +0000
> @@ -26,6 +26,12 @@
>  Dialog {
>      id: root
>  
> +    title: i18n.tr("Edit tags")
> +    text: tags.count == 0 ? noTagsText : haveTagsText
> +
> +    property string noTagsText: i18n.tr("Enter a tag name to attach it to the note.")
> +    property string haveTagsText: i18n.tr("Enter a tag name or select one from the list to attach it to the note.")
> +
>      property var note
>      property int pageHeight
>  
> @@ -35,42 +41,91 @@
>          id: tags
>      }
>  
> +    SortFilterModel {
> +        id: tagsSortFilterModel
> +        model: tags
> +        filter.property: "name"
> +        filter.pattern: RegExp(textField.text)
> +    }
> +
>      RowLayout {
>          Layout.preferredWidth: parent.width - units.gu(2)
>          Layout.alignment: Qt.AlignHCenter
> +        z: 2
>  
> -        TextField {
> -            id: textField
> +        Item {
>              Layout.fillWidth: true
> -            placeholderText: i18n.tr("Create a new tag")
> -
> -            onAccepted: accept();
> -
> -            function accept() {
> -                var tagName = text;
> -                text = '';
> -
> -                // Check if the tag exists
> -                for (var i=0; i < tags.count; i++) {
> -                    var tag = tags.tag(i);
> -                    if (tag.name == tagName) {
> -                        // The tag exists, check if is already selected: if it is,
> -                        // do nothing, otherwise add to tags of the note
> -                        if (note.tagGuids.indexOf(tag.guid) === -1) {
> -                            note.tagGuids.push(tag.guid);
> -                        }
> -                        return;
> -                    }
> -                }
> -
> -                var newTag = NotesStore.createTag(tagName);
> -                print("tag created", newTag.name, "appending to note");
> -                note.tagGuids.push(newTag.guid)
> +            Layout.preferredHeight: okButton.height
> +
> +            TextField {
> +                id: textField
> +                placeholderText: i18n.tr("Tag name")
> +                anchors.fill: parent
> +
> +                onAccepted: accept();
> +
> +                function accept() {
> +                    var tagName = text;
> +                    text = '';
> +
> +                    // Check if the tag exists
> +                    for (var i=0; i < tags.count; i++) {
> +                        var tag = tags.tag(i);
> +                        if (tag.name == tagName) {
> +                            // The tag exists, check if is already selected: if it is,
> +                            // do nothing, otherwise add to tags of the note
> +                            if (note.tagGuids.indexOf(tag.guid) === -1) {
> +                                note.tagGuids.push(tag.guid);
> +                            }
> +                            return;
> +                        }
> +                    }
> +
> +                    var newTag = NotesStore.createTag(tagName);
> +                    print("tag created", newTag.name, "appending to note");
> +                    note.tagGuids.push(newTag.guid)
> +                }
> +
> +            }
> +
> +            Rectangle {

Shoudln't be UbuntuShape?

> +                x: textField.x
> +                y: textField.y + textField.height

What's wrong with anchors?

> +                width: textField.width
> +                color: "white"
> +                border.width: units.dp(1)
> +                border.color: "black"
> +                height: Math.min(5, tagsListView.count) * units.gu(4)
> +                visible: textField.text.length > 0

Cool, but I'd prefer to have it visible only when there is focus on the textfield: I start to type, then I want to scroll already existing tags: when I press out of the textfield, I expect the popup disappears

> +
> +                ListView {
> +                    id: tagsListView
> +                    anchors.fill: parent
> +                    model: tagsSortFilterModel
> +                    clip: true
> +
> +                    delegate: Empty {
> +                        height: units.gu(4)
> +                        Label {
> +                            anchors.fill: parent
> +                            anchors.margins: units.gu(1)
> +                            text: model.name
> +                            color: textField.text === model.name ? UbuntuColors.orange : "black"
> +                        }
> +
> +                        onClicked: {
> +                            textField.text = model.name
> +                            textField.accept()

I think this is a bit confusing without an animation that confirm you selected a new tag. Maybe you just drop this line, or add a 'tag added' label

> +                        }
> +                    }
> +                }
>              }
>          }
>  
> +
>          Button {
> -            text: i18n.tr("Create tag")
> +            id: okButton
> +            text: i18n.tr("OK")
>              color: UbuntuColors.orange
>              enabled: textField.text.replace(/\s+/g, '') !== ''; // Not only whitespaces!
>              onClicked: textField.accept()
> @@ -86,7 +141,7 @@
>          currentlyExpanded: true
>          multiSelection: true
>  
> -        containerHeight: Math.min(root.pageHeight - textField.height - closeButton.height - units.gu(12), tags.count * itemHeight)
> +        containerHeight: Math.min(root.pageHeight / 3, tags.count * itemHeight)
>  
>          model: tags
>  
> @@ -117,7 +172,7 @@
>  
>          color: UbuntuColors.orange
>  
> -        text: i18n.tr("Done")
> +        text: i18n.tr("Close")
>  
>          onClicked: {
>              root.done();
> 
> === modified file 'src/app/qml/reminders.qml'
> --- src/app/qml/reminders.qml	2015-06-01 09:56:30 +0000
> +++ src/app/qml/reminders.qml	2015-06-11 19:39:25 +0000
> @@ -383,7 +383,7 @@
>          onAuthenticated: {
>              EvernoteConnection.token = reply.AccessToken;
>              print("token is:", EvernoteConnection.token)
> -            if (NetworkingStatus.online) {
> +            if (NetworkingStatus.Online) {
>                  EvernoteConnection.connectToEvernote();
>              }
>          }
> 
> === modified file 'src/app/qml/ui/EditNoteView.qml'
> --- src/app/qml/ui/EditNoteView.qml	2015-05-30 16:18:42 +0000
> +++ src/app/qml/ui/EditNoteView.qml	2015-06-11 19:39:25 +0000
> @@ -647,7 +647,7 @@
>              RtfButton {
>                  iconName: "tick"
>                  // TRANSLATORS: Button to close the edit mode
> -                text: i18n.tr("Done")
> +                text: i18n.tr("Close")
>                  height: parent.height
>                  iconColor: UbuntuColors.green
>                  onClicked: {
> 
> === modified file 'src/libqtevernote/notesstore.cpp'
> --- src/libqtevernote/notesstore.cpp	2015-03-30 19:01:49 +0000
> +++ src/libqtevernote/notesstore.cpp	2015-06-11 19:39:25 +0000
> @@ -478,6 +478,12 @@
>  
>  Tag* NotesStore::createTag(const QString &name)
>  {
> +    foreach (Tag *tag, m_tags) {
> +        if (tag->name() == name) {
> +            return tag;
> +        }
> +    }
> +
>      QString newGuid = QUuid::createUuid().toString();
>      newGuid.remove("{").remove("}");
>      Tag *tag = new Tag(newGuid, 1, this);
> 


-- 
https://code.launchpad.net/~mzanetti/reminders-app/improve-edit-tags/+merge/261779
Your team Ubuntu Reminders app developers is subscribed to branch lp:reminders-app.