Merge lp:~ahayzen/ubuntu-weather-app/reboot-day-delegate-expand into lp:ubuntu-weather-app
Status: | Merged |
---|---|
Approved by: | Victor Thompson on 2015-04-30 |
Approved revision: | 35 |
Merged at revision: | 37 |
Proposed branch: | lp:~ahayzen/ubuntu-weather-app/reboot-day-delegate-expand |
Merge into: | lp:ubuntu-weather-app |
Diff against target: |
1203 lines (+1008/-25) 9 files modified
app/components/DayDelegate.qml (+155/-5) app/components/ForecastDetailsDelegate.qml (+49/-0) app/graphics/extended-information_chance-of-rain.svg (+153/-0) app/graphics/extended-information_humidity.svg (+147/-0) app/graphics/extended-information_uv-level.svg (+270/-0) app/graphics/extended-information_wind.svg (+155/-0) app/ui/HomePage.qml (+23/-8) app/ui/LocationPane.qml (+27/-11) po/com.ubuntu.weather.pot (+29/-1) |
To merge this branch: | bzr merge lp:~ahayzen/ubuntu-weather-app/reboot-day-delegate-expand |
Related bugs: | |
Related blueprints: |
Weather App Reboot
(High)
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Victor Thompson | 2015-04-06 | Approve on 2015-04-30 | |
Ubuntu Phone Apps Jenkins Bot | continuous-integration | Approve on 2015-04-29 | |
Review via email:
|
Commit message
* First iteration of expandable day delegate
Description of the change
* First iteration of expandable day delegate
This does not include a working "overview text", sunrise, sunset, pollen levels. These will hopefully come in a second mp :)
Victor Thompson (vthompson) wrote : | # |
This works very well! Would it make sense to close all the expanded days when the user switches to a different location, or if they go into the Settings or Locations pages?
Martin Borho (martin-borho) wrote : | # |
In the old app every location was "resetted" after leaving, so the view was back at the top and all other changes in the view were dismissed when re-entering.
I would even say, it would be ok to have only day forecast expanded, in the location itself.
Otherwise, well done!
Victor Thompson (vthompson) wrote : | # |
I agree--maybe only one should be expanded at a time. The spec makes it seem like this is the case (mostly because the expanded data takes up most of the screen). The reasoning for leaving multiple expanded was because since this data is more compact it might be nice to allow multiple to be expanded. However, you lose most of the benefits of having multiple expanded if you auto-scroll to the currently expanded item (which looks slick, btw).
Nekhelesh Ramananthan (nik90) wrote : | # |
Yeah it would be best to have only one listitem expanded at a time. I noticed a issue in the background color of the hourly forecast [1], however at the time I wasn't sure if this was a issue in trunk or in this branch. So I fixed it along with some other cleanups in [2]. So ignore that issue while reviewing this branch as merging my branch would fix it.
[1] https:/
[2] https:/
Victor Thompson (vthompson) wrote : | # |
In addition to what we decided above (only allow for 1 expanded day and auto-collapse each when the view changes), I've noticed that when a different location is selected from the Locations page, the transition is incredibly slow on the device. This only appears to be an issue with this branch and not trunk/reboot.
Victor Thompson (vthompson) wrote : | # |
Also, this mp seems to make the app's background color non-white on vivid, which makes some of the icons look odd (it's not as visible in this screenshot, however): http://
Alan Pope πΊπ§π± π¦ (popey) wrote : | # |
+1 to only having one expanded at a time.
Martin Borho (martin-borho) wrote : | # |
Design said, it would be ok to stay with the current way after changing the location. So no forced reset in the ui without user input.
We can revisit it later on, if it doesn't work out..
Besides that, only one daily forecast can/should be expanded. As intended.
- 32. By Andrew Hayzen on 2015-04-26
-
* Merge of /reboot
- 33. By Andrew Hayzen on 2015-04-26
-
* Remove scrolling Flickable as it causes issues when collapsing and expanding delegates
* Simplifed how height of Flickable is calculated
* Only one delegate can now be in the expanded state
* All delegates are collaped when the HomePage.qml changes to invisible or the currently shown location changes
Andrew Hayzen (ahayzen) wrote : | # |
I've removed the auto scroll for now as it was becoming messy when you had one item expanded and you go to click on another, as it collapses one and expands the other the height then moves around a lot, let me know if you do infact want this in.
I've also changed the way the flickable's height is calculated, let me know if it is slightly too tight to the bottom item.
Regarding collapsing delegates I went with collapsing when either of the following occur:
- Another delegate is expanded
- The selected location changes
- The HomePage becomes invisible (another page pushed on top in the stack)
Let me know if you want any changes regarding this behaviour :)
PASSED: Continuous integration, rev:33
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Victor Thompson (vthompson) wrote : | # |
See inline comment. I think the rest looks good.
Victor Thompson (vthompson) wrote : | # |
The offset at the bottom seems more like 3GU. But I agree--not sure where it's coming from.
- 34. By Andrew Hayzen on 2015-04-29
-
* Change to 3GU
FAILED: Continuous integration, rev:34
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 35. By Andrew Hayzen on 2015-04-29
-
* Merge of /reboot
PASSED: Continuous integration, rev:35
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Victor Thompson (vthompson) wrote : | # |
LGTM, let's land this and iterate as we go along.
PASSED: Continuous integration, rev:31 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- ci/68/ 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- utopic- amd64-ci/ 38 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- vivid-amd64- ci/68
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild: 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- ci/68/rebuild
http://