Merge lp:~rpadovani/ubuntu-weather-app/1200600 into lp:ubuntu-weather-app/obsolete.trunk

Proposed by Riccardo Padovani on 2013-07-12
Status: Merged
Approved by: Martin Borho on 2013-07-14
Approved revision: 62
Merged at revision: 62
Proposed branch: lp:~rpadovani/ubuntu-weather-app/1200600
Merge into: lp:ubuntu-weather-app/obsolete.trunk
Diff against target: 33 lines (+12/-0)
1 file modified
components/AddLocationPage.qml (+12/-0)
To merge this branch: bzr merge lp:~rpadovani/ubuntu-weather-app/1200600
Reviewer Review Type Date Requested Status
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve on 2013-07-15
Nekhelesh Ramananthan (community) Approve on 2013-07-14
Martin Borho 2013-07-12 Approve on 2013-07-13
Review via email:

Commit message

Added "No city" in city list when no city is found.
See #1200600

Description of the change

Added "No city" in city list when no city is found.
See #1200600

To post a comment you must log in.
Martin Borho (martin-borho) wrote :

Thanks for the fix, that was fast!

But one request:

By using only "if (citiesModel.count < 1) {" in the onMessage handler instead of "if (!number) {", you could remove the "number" var. Leads to less code.


review: Needs Fixing
61. By Riccardo Padovani on 2013-07-13

lightened the control for no city message

Riccardo Padovani (rpadovani) wrote :

Good point, done :)

Nekhelesh Ramananthan (nik90) wrote :

@Martin, Riccardo: I do not it is necessary to provide a width to the Label? Also I was told wherever possible try using anchors instead of hard coding width or height.

review: Needs Fixing
Martin Borho (martin-borho) wrote :

@Nekhelesh, Riccardo: I guess the design of the label is likely to be changed anyways when the visual design is ready. So for me it's ok for the moment.

But Riccardo can change it, if he wants to?

Nekhelesh Ramananthan (nik90) wrote :

@Martin: Yes the design might change in the future, however my point was that you do not need to specify the width of the label. Since Riccardo has specified the anchors in the page, that alone is sufficient.

Label {
 text: "Sometext"
 anchors.centerin: parent

This code alone would do it.

review: Needs Fixing
Martin Borho (martin-borho) wrote :

@Nekhelesh: ah, got you! your're right of course.

62. By Riccardo Padovani on 2013-07-13

Delete width of label

Riccardo Padovani (rpadovani) wrote :

Ok, done it.
Sorry for the error, but I have not figured everything yet.

Martin Borho (martin-borho) wrote :

Fine, thanks Riccardo!

review: Approve
Nekhelesh Ramananthan (nik90) wrote :

lgtm now. Ready for merge.

review: Approve
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'components/AddLocationPage.qml'
2--- components/AddLocationPage.qml 2013-07-03 19:39:29 +0000
3+++ components/AddLocationPage.qml 2013-07-13 13:37:26 +0000
4@@ -28,10 +28,15 @@
5 listView.visible = true
6 messageObject.result.locations.forEach(function(loc) {
7 citiesModel.append(loc);
8+ noCityError.visible = false
9 });
10 } else {
11 console.log(messageObject.error.msg+" / "+messageObject.error.request.url)
12 }
13+ if (!citiesModel.count) {
14+ noCityError.visible = true
15+ noCityError.text ="No location was found for ") + locationString.text
16+ }
17 }
18 }
20@@ -74,6 +79,13 @@
21 width: parent.width
22 height:
23 color: "transparent"
24+ Label {
25+ id: noCityError
26+ visible: false
27+ anchors.centerIn: parent;
28+ font.pixelSize: 18
29+ }
31 ListView {
32 id: listView;
33 objectName: "SearchResultList"


People subscribed via source and target branches