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
=== modified file 'qml/Dash/DashNavigationList.qml'
--- qml/Dash/DashNavigationList.qml 2014-10-23 11:59:22 +0000
+++ qml/Dash/DashNavigationList.qml 2015-07-06 08:19:12 +0000
@@ -16,6 +16,7 @@
1616
17import QtQuick 2.217import QtQuick 2.2
18import Ubuntu.Components 1.118import Ubuntu.Components 1.1
19import Ubuntu.Components.ListItems 0.1 as ListItem
19import "../Components"20import "../Components"
2021
21Item {22Item {
@@ -58,13 +59,9 @@
58 id: column59 id: column
59 width: parent.width60 width: parent.width
6061
61 // TODO: check if SDK ListItems could be used here62 ListItem.Standard {
62 // and if not make them be useful since this is a quite common pattern
63
64 AbstractButton {
65 id: backButton63 id: backButton
66 objectName: "backButton"64 objectName: "backButton"
67 width: parent.width
68 visible: navigation && !navigation.isRoot || false65 visible: navigation && !navigation.isRoot || false
69 height: itemHeight66 height: itemHeight
7067
@@ -97,25 +94,11 @@
97 maximumLineCount: 294 maximumLineCount: 2
98 elide: Text.ElideMiddle95 elide: Text.ElideMiddle
99 }96 }
100
101 Rectangle {
102 anchors {
103 bottom: parent.bottom
104 left: parent.left
105 right: parent.right
106 leftMargin: units.gu(2)
107 rightMargin: units.gu(2)
108 }
109 color: root.foregroundColor
110 opacity: 0.2
111 height: units.dp(1)
112 }
113 }97 }
11498
115 AbstractButton {99 ListItem.Standard {
116 id: allButton100 id: allButton
117 objectName: "allButton"101 objectName: "allButton"
118 width: parent.width
119 visible: navigation && (!navigation.isRoot || (!navigation.hidden && root.currentNavigation && !root.currentNavigation.isRoot && root.currentNavigation.parentNavigationId == navigation.navigationId)) || false102 visible: navigation && (!navigation.isRoot || (!navigation.hidden && root.currentNavigation && !root.currentNavigation.isRoot && root.currentNavigation.parentNavigationId == navigation.navigationId)) || false
120 height: itemHeight103 height: itemHeight
121104
@@ -135,29 +118,17 @@
135 elide: Text.ElideMiddle118 elide: Text.ElideMiddle
136 }119 }
137120
138 Rectangle {
139 anchors {
140 bottom: parent.bottom
141 left: parent.left
142 right: parent.right
143 leftMargin: units.gu(2)
144 rightMargin: units.gu(2)
145 }
146 color: root.foregroundColor
147 opacity: 0.2
148 height: units.dp(1)
149 }
150
151 onClicked: root.allNavigationClicked();121 onClicked: root.allNavigationClicked();
152 }122 }
153123
154 Repeater {124 Repeater {
155 model: navigation && navigation.loaded ? navigation : null125 model: navigation && navigation.loaded ? navigation : null
156 clip: true126 clip: true
157 delegate: AbstractButton {127 delegate: ListItem.Standard {
158 objectName: root.objectName + "child" + index128 objectName: root.objectName + "child" + index
159 height: root.itemHeight129 height: root.itemHeight
160 width: root.width130 showDivider: index != navigation.count - 1
131 selected: isActive
161132
162 onClicked: root.enterNavigation(navigationId, hasChildren)133 onClicked: root.enterNavigation(navigationId, hasChildren)
163134
@@ -189,20 +160,6 @@
189 color: root.foregroundColor160 color: root.foregroundColor
190 visible: hasChildren || isActive161 visible: hasChildren || isActive
191 }162 }
192
193 Rectangle {
194 anchors {
195 bottom: parent.bottom
196 left: parent.left
197 right: parent.right
198 leftMargin: units.gu(2)
199 rightMargin: units.gu(2)
200 }
201 color: root.foregroundColor
202 opacity: 0.1
203 height: units.dp(1)
204 visible: index != navigation.count - 1
205 }
206 }163 }
207 }164 }
208 }165 }

Subscribers

People subscribed via source and target branches