Unity 8

Merge lp:~feng-kylin/unity8/AddTouchStateOnNavigation into lp:unity8

  1. AddTouchStateOnNavigation
  2. Merge into trunk
Proposed by handsome_feng
Status: Merged
Approved by: Albert Astals Cid
Approved revision: 1826
Merged at revision: 1875
Proposed branch: lp:~feng-kylin/unity8/AddTouchStateOnNavigation
Merge into: lp:unity8
Diff against target: 107 lines (+6/-49)
1 file modified
qml/Dash/DashNavigationList.qml (+6/-49)
To merge this branch: bzr merge lp:~feng-kylin/unity8/AddTouchStateOnNavigation
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Needs Fixing
Albert Astals Cid (community) Abstain
Mirco Müller (community) Abstain
Review via email: mp+262653@code.launchpad.net

Commit message

Added touch state on navigation.

Description of the change

Added touch state on navigation.

* Are there any related MPs required for this MP to build/function as expected? Please list.

no

 * Did you perform an exploratory manual test run of your code change and any related functionality?

yes

 * Did you make sure that your branch does not contain spurious tags?

yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

n/a

 * If you changed the UI, has there been a design review?

no

To post a comment you must log in.
Revision history for this message
Mirco Müller (macslow) :
Revision history for this message
Mirco Müller (macslow) wrote :

See inline-comments.

review: Needs Fixing
Revision history for this message
Mirco Müller (macslow) wrote :

Doh... never mind... just saw the Rectangle with height = units.dp(1) which acts are the divider.

I've still to run and see the change in action. Once that's done I'll approve it as the code looks fine to me.

review: Abstain
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1823
http://jenkins.qa.ubuntu.com/job/unity8-ci/5934/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/256
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/214
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/215
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/176
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/256
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/256/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21645

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5934/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Please add this checklist[1] to the description field of this merge proposal.

[1] - https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8

Revision history for this message
Mirco Müller (macslow) wrote :

Have a look at this photo...

* http://macslow.org/compare-dash-navigation.jpg

On the left it shows your branch running on a Nexus4, while on the left you see a bq Aquaris E4.5 running stock unity8. Looking at the photo provided by Design with the bug...

* https://launchpadlibrarian.net/187741760/touch-estate-example.jpg

... the left and right edges look cut off in your version? Is that ok with Design? It feels weird to me. Please check with them and have one of them comment here.

review: Needs Information
Revision history for this message
handsome_feng (feng-kylin) wrote :

> Have a look at this photo...
>
> * http://macslow.org/compare-dash-navigation.jpg
>
> On the left it shows your branch running on a Nexus4, while on the left you
> see a bq Aquaris E4.5 running stock unity8. Looking at the photo provided by
> Design with the bug...
>
> * https://launchpadlibrarian.net/187741760/touch-estate-example.jpg
>
> ... the left and right edges look cut off in your version? Is that ok with
> Design? It feels weird to me. Please check with them and have one of them
> comment here.

I am not sure whether the white space of both edges is needed or not. But since
the photo above is provided by the Designer, I will make it as the given example.

Revision history for this message
Albert Astals Cid (aacid) wrote :

Please clean your tags as described in https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8

review: Needs Fixing
Revision history for this message
handsome_feng (feng-kylin) wrote :

Seems the tags did not pushed to my branch...
sorry, can anyone tell me how to push it after I delete the
spurious tags ? Thank you in advance!

Revision history for this message
Albert Astals Cid (aacid) wrote :

Just run
 strip-tags.py lp:~feng-kylin/unity8/AddTouchStateOnNavigation

Revision history for this message
handsome_feng (feng-kylin) wrote :

> Just run
> strip-tags.py lp:~feng-kylin/unity8/AddTouchStateOnNavigation

Thank you! done.

Revision history for this message
Albert Astals Cid (aacid) wrote :

Tags are clean i'll leave the rest to Mirco

review: Abstain (tags are clean)
Revision history for this message
Mirco Müller (macslow) wrote :

Ok, looking good now.

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes, see http://macslow.org/compare-dash-navigation-2.jpg

* Did CI run pass? If not, please explain why.
No, because there were unrelated autopilot-failures.

* Did you make sure that the branch does not contain spurious tags?
Yes, all clean.

review: Approve
Revision history for this message
Albert Astals Cid (aacid) wrote :

I'd say the "allButton" also needs a selected.

I.e.
  make tryDash
  Click on root, rootChild2, allmiddle2
  Now open again the departments again by clicking on middle2
  Any reason allmiddle2 doesn't have a checkmark?

review: Needs Fixing
Revision history for this message
handsome_feng (feng-kylin) wrote :

> I'd say the "allButton" also needs a selected.
>
> I.e.
> make tryDash
> Click on root, rootChild2, allmiddle2
> Now open again the departments again by clicking on middle2
> Any reason allmiddle2 doesn't have a checkmark?

I am really not sure about this, I just followed the previously design.

Revision history for this message
Mirco Müller (macslow) wrote :

> > I'd say the "allButton" also needs a selected.
> >
> > I.e.
> > make tryDash
> > Click on root, rootChild2, allmiddle2
> > Now open again the departments again by clicking on middle2
> > Any reason allmiddle2 doesn't have a checkmark?
>
> I am really not sure about this, I just followed the previously design.

I tend to agree, since the initial UX-bug https://bugs.launchpad.net/ubuntu-ux/+bug/1383213, only mentions the grey colour for the selected state of an entry being missing and nothing about the checkmark for the "All"-category.

review: Abstain
Revision history for this message
Albert Astals Cid (aacid) wrote :

Ok

review: Abstain
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1826
http://jenkins.qa.ubuntu.com/job/unity8-ci/6046/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/348
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-wily/76
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/284
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/285
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/252
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/348
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/348/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21865

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/6046/rebuild

review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'qml/Dash/DashNavigationList.qml'
2--- qml/Dash/DashNavigationList.qml 2014-10-23 11:59:22 +0000
3+++ qml/Dash/DashNavigationList.qml 2015-07-06 08:19:12 +0000
4@@ -16,6 +16,7 @@
5
6 import QtQuick 2.2
7 import Ubuntu.Components 1.1
8+import Ubuntu.Components.ListItems 0.1 as ListItem
9 import "../Components"
10
11 Item {
12@@ -58,13 +59,9 @@
13 id: column
14 width: parent.width
15
16- // TODO: check if SDK ListItems could be used here
17- // and if not make them be useful since this is a quite common pattern
18-
19- AbstractButton {
20+ ListItem.Standard {
21 id: backButton
22 objectName: "backButton"
23- width: parent.width
24 visible: navigation && !navigation.isRoot || false
25 height: itemHeight
26
27@@ -97,25 +94,11 @@
28 maximumLineCount: 2
29 elide: Text.ElideMiddle
30 }
31-
32- Rectangle {
33- anchors {
34- bottom: parent.bottom
35- left: parent.left
36- right: parent.right
37- leftMargin: units.gu(2)
38- rightMargin: units.gu(2)
39- }
40- color: root.foregroundColor
41- opacity: 0.2
42- height: units.dp(1)
43- }
44 }
45
46- AbstractButton {
47+ ListItem.Standard {
48 id: allButton
49 objectName: "allButton"
50- width: parent.width
51 visible: navigation && (!navigation.isRoot || (!navigation.hidden && root.currentNavigation && !root.currentNavigation.isRoot && root.currentNavigation.parentNavigationId == navigation.navigationId)) || false
52 height: itemHeight
53
54@@ -135,29 +118,17 @@
55 elide: Text.ElideMiddle
56 }
57
58- Rectangle {
59- anchors {
60- bottom: parent.bottom
61- left: parent.left
62- right: parent.right
63- leftMargin: units.gu(2)
64- rightMargin: units.gu(2)
65- }
66- color: root.foregroundColor
67- opacity: 0.2
68- height: units.dp(1)
69- }
70-
71 onClicked: root.allNavigationClicked();
72 }
73
74 Repeater {
75 model: navigation && navigation.loaded ? navigation : null
76 clip: true
77- delegate: AbstractButton {
78+ delegate: ListItem.Standard {
79 objectName: root.objectName + "child" + index
80 height: root.itemHeight
81- width: root.width
82+ showDivider: index != navigation.count - 1
83+ selected: isActive
84
85 onClicked: root.enterNavigation(navigationId, hasChildren)
86
87@@ -189,20 +160,6 @@
88 color: root.foregroundColor
89 visible: hasChildren || isActive
90 }
91-
92- Rectangle {
93- anchors {
94- bottom: parent.bottom
95- left: parent.left
96- right: parent.right
97- leftMargin: units.gu(2)
98- rightMargin: units.gu(2)
99- }
100- color: root.foregroundColor
101- opacity: 0.1
102- height: units.dp(1)
103- visible: index != navigation.count - 1
104- }
105 }
106 }
107 }

Subscribers

People subscribed via source and target branches