Merge lp:~pkunal-parmar/ubuntu-calendar-app/yearview_fix into lp:ubuntu-calendar-app

Proposed by Kunal Parmar
Status: Merged
Approved by: Olivier Tilloy
Approved revision: 149
Merged at revision: 150
Proposed branch: lp:~pkunal-parmar/ubuntu-calendar-app/yearview_fix
Merge into: lp:ubuntu-calendar-app
Diff against target: 173 lines (+44/-64)
3 files modified
MonthComponent.qml (+13/-29)
ViewHeader.qml (+4/-2)
YearView.qml (+27/-33)
To merge this branch: bzr merge lp:~pkunal-parmar/ubuntu-calendar-app/yearview_fix
Reviewer Review Type Date Requested Status
Olivier Tilloy (community) Approve
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve
Review via email: mp+190016@code.launchpad.net

Commit message

YearView changed to use GridView
MonthComponent is creating hilight only when necessary

Description of the change

YearView changed to use GridView
MonthComponent is creating hilight only when necessary

To post a comment you must log in.
140. By Kunal Parmar

newlines removed

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
141. By Kunal Parmar

reused view header

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

When I click on any month in the year view, I’m getting the following error:

    YearView.qml:79: ReferenceError: monthDate is not defined

review: Needs Fixing
Revision history for this message
Olivier Tilloy (osomon) wrote :

52 + property bool shouldCreateHighlight: date.isSameDay(DateExt.today()) && isCurrentMonth
53 + property var hightlightObj;
54 +
55 + onShouldCreateHighlightChanged: {
56 + if( shouldCreateHighlight ) {
57 + hightlightObj = highLightComp.createObject(dateRootItem);
58 + hightlightObj.z = hightlightObj.z -1;
59 + } else {
60 + if( hightlightObj) {
61 + hightlightObj.destroy();
62 + }
63 + }

Please use a Loader element instead of manually instantiating/destroying objects.

review: Needs Fixing
Revision history for this message
Olivier Tilloy (osomon) wrote :

128 + flow: Grid.LeftToRight

Grid.LeftToRight is the default value, so this is useless.

review: Needs Fixing
Revision history for this message
Olivier Tilloy (osomon) wrote :

169 + model: 6/*rows*/ * 2 /*columns*/

or, simpler:

    model: 12 /* months in a year */

Revision history for this message
Olivier Tilloy (osomon) wrote :

178 + width: parent.width - units.gu(1)
179 + height: parent.height - units.gu(1)
180 + anchors.centerIn: parent

Better to use "anchors.fill: parent" and "anchors.margins: units.gu(0.5)" in that case.

142. By Kunal Parmar

monthDate is not defined fixed

143. By Kunal Parmar

flow: Grid.LeftToRight

144. By Kunal Parmar

 model: 12 /* months in a year */

145. By Kunal Parmar

layouting fixed for monthComponent

146. By Kunal Parmar

Loader used instead of manual creation of object

Revision history for this message
Kunal Parmar (pkunal-parmar) wrote :

Thanks, I addressed your comments, Pls have a look again.

Revision history for this message
Olivier Tilloy (osomon) wrote :

> Thanks, I addressed your comments, Pls have a look again.

Looks like you forgot to push your changes to the branch, I’m not seeing any updates.

Revision history for this message
Kunal Parmar (pkunal-parmar) wrote :

> > Thanks, I addressed your comments, Pls have a look again.
>
> Looks like you forgot to push your changes to the branch, I’m not seeing any
> updates.

Someting is wrong with bzr, when I use bzr push, it it pushing changes to different branch.
But now this branch should have my recent changes. Thanks..

Revision history for this message
Kunal Parmar (pkunal-parmar) wrote :
Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

> Someting is wrong with bzr, when I use bzr push, it it pushing changes to
> different branch.

If you issue `bzr info`, it will show where it will push by default. You can change the default push location with `bzr push --remember $LOCATION`

Revision history for this message
Olivier Tilloy (osomon) wrote :

46 + property bool shouldCreateHighlight: date.isSameDay(DateExt.today()) && isCurrentMonth
47 +
48 + onShouldCreateHighlightChanged: {
49 + if( shouldCreateHighlight ) {
50 + highlightLoader.sourceComponent = highLightComp;
51 + } else {
52 + //unloading the highlight
53 + highlightLoader.sourceComponent = undefined
54 + }
55 + }

This is better, but it can be taken one step further to make it fully declarative: remove entirely the shouldCreateHighlight property, and do this:

    Loader {
        sourceComponent: date.isSameDay(DateExt.today()) && isCurrentMonth ? highLightComp : undefined
        […]
    }

(and you can then remove the 'highlightLoader' id which will be unused).

review: Needs Fixing
Revision history for this message
Olivier Tilloy (osomon) wrote :

78 + id: highLightRect

Please also remove this id which is unused.

147. By Kunal Parmar

id: highLightRect removed

148. By Kunal Parmar

using declarative way to load a loader

Revision history for this message
Kunal Parmar (pkunal-parmar) wrote :

> > Someting is wrong with bzr, when I use bzr push, it it pushing changes to
> > different branch.
>
> If you issue `bzr info`, it will show where it will push by default. You can
> change the default push location with `bzr push --remember $LOCATION`

Thanks, this helped

149. By Kunal Parmar

merge from trunk

Revision history for this message
Kunal Parmar (pkunal-parmar) wrote :

> 46 + property bool shouldCreateHighlight:
> date.isSameDay(DateExt.today()) && isCurrentMonth
> 47 +
> 48 + onShouldCreateHighlightChanged: {
> 49 + if( shouldCreateHighlight ) {
> 50 + highlightLoader.sourceComponent = highLightComp;
> 51 + } else {
> 52 + //unloading the highlight
> 53 + highlightLoader.sourceComponent = undefined
> 54 + }
> 55 + }
>
> This is better, but it can be taken one step further to make it fully
> declarative: remove entirely the shouldCreateHighlight property, and do this:
>

Done, please have another look
> Loader {
> sourceComponent: date.isSameDay(DateExt.today()) && isCurrentMonth ?
> highLightComp : undefined
> […]
> }
>
> (and you can then remove the 'highlightLoader' id which will be unused).

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

Looks good now, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'MonthComponent.qml'
2--- MonthComponent.qml 2013-09-04 13:37:10 +0000
3+++ MonthComponent.qml 2013-10-09 23:35:38 +0000
4@@ -35,30 +35,11 @@
5 anchors.fill: parent
6 spacing: units.gu(1.5)
7
8- Item{
9+ ViewHeader{
10 id: monthHeader
11- width: parent.width
12- height: monthLabel.height
13-
14- Label{
15- id: monthLabel
16- fontSize: monthLabelFontSize
17- text: Qt.locale().standaloneMonthName(root.monthDate.getMonth())
18- anchors.leftMargin: units.gu(1)
19- anchors.left: parent.left
20- //color:"white"
21- anchors.verticalCenter: parent.verticalCenter
22- }
23-
24- Label{
25- id: yearLabel
26- fontSize: yearLabelFontSize
27- text: root.monthDate.getFullYear()
28- anchors.right: parent.right
29- anchors.rightMargin: units.gu(1)
30- color:"#AEA79F"
31- anchors.verticalCenter: parent.verticalCenter
32- }
33+ date: root.monthDate
34+ monthLabelFontSize: root.monthLabelFontSize
35+ yearLabelFontSize: root.yearLabelFontSize
36 }
37
38 Row{
39@@ -106,15 +87,11 @@
40 width: parent.width / 7;
41 height: parent.height / parent.weekCount
42
43- UbuntuShape{
44- id: highLightRect
45-
46+ Loader {
47 width: parent.width
48 height: width
49 anchors.centerIn: parent
50-
51- color: "white"
52- visible: date.isSameDay(DateExt.today()) && isCurrentMonth
53+ sourceComponent: date.isSameDay(DateExt.today()) && isCurrentMonth ? highLightComp : undefined
54 }
55
56 Label{
57@@ -156,4 +133,11 @@
58 color: "#AEA79F"
59 }
60 }
61+
62+ Component{
63+ id: highLightComp
64+ UbuntuShape{
65+ color: "white"
66+ }
67+ }
68 }
69
70=== modified file 'ViewHeader.qml'
71--- ViewHeader.qml 2013-09-16 14:04:37 +0000
72+++ ViewHeader.qml 2013-10-09 23:35:38 +0000
73@@ -7,10 +7,12 @@
74 height: monthLabel.height
75
76 property var date;
77+ property string monthLabelFontSize: "x-large"
78+ property string yearLabelFontSize: "large"
79
80 Label{
81 id: monthLabel
82- fontSize: "large"
83+ fontSize: monthLabelFontSize
84 text: Qt.locale().standaloneMonthName(date.getMonth())
85 anchors.leftMargin: units.gu(1)
86 anchors.left: parent.left
87@@ -20,7 +22,7 @@
88
89 Label{
90 id: yearLabel
91- fontSize: "medium"
92+ fontSize: yearLabelFontSize
93 text: date.getFullYear()
94 anchors.right: parent.right
95 anchors.rightMargin: units.gu(1)
96
97=== modified file 'YearView.qml'
98--- YearView.qml 2013-09-16 14:04:01 +0000
99+++ YearView.qml 2013-10-09 23:35:38 +0000
100@@ -29,7 +29,7 @@
101 property var startYear: getDateFromYear(currentYear.getFullYear()-1);
102 }
103
104- delegate: Flickable{
105+ delegate: GridView{
106 id: yearView
107 clip: true
108
109@@ -48,38 +48,32 @@
110
111 width: parent.width
112 height: parent.height
113-
114- contentHeight: yearGrid.height + units.gu(2)
115- contentWidth: width
116-
117- Grid{
118- id: yearGrid
119- rows: 6
120- columns: 2
121-
122- anchors.top: parent.top
123- anchors.topMargin: units.gu(1.5)
124-
125- width: parent.width - ((columns-1) * yearGrid.spacing)
126- spacing: units.gu(2)
127- anchors.horizontalCenter: parent.horizontalCenter
128-
129- Repeater{
130- model: yearGrid.rows * yearGrid.columns
131- delegate: MonthComponent{
132- monthDate: new Date(yearView.year.getFullYear(),index,1,0,0,0,0)
133- width: (parent.width - units.gu(2))/2
134- height: width * 1.5
135- dayLabelFontSize:"x-small"
136- dateLabelFontSize: "medium"
137- monthLabelFontSize: "medium"
138- yearLabelFontSize: "small"
139-
140- MouseArea{
141- anchors.fill: parent
142- onClicked: {
143- root.monthSelected(monthDate);
144- }
145+ anchors.top: parent.top
146+
147+ cellWidth: width/2
148+ cellHeight: cellWidth * 1.2
149+
150+ model: 12 /* months in a year */
151+ snapMode: GridView.SnapOneRow
152+ delegate: Item {
153+ width: yearView.cellWidth
154+ height: yearView.cellHeight
155+
156+ MonthComponent{
157+ id: monthComponent
158+ monthDate: new Date(yearView.year.getFullYear(),index,1,0,0,0,0)
159+ anchors.fill: parent
160+ anchors.margins: units.gu(0.5)
161+
162+ dayLabelFontSize:"x-small"
163+ dateLabelFontSize: "medium"
164+ monthLabelFontSize: "medium"
165+ yearLabelFontSize: "small"
166+
167+ MouseArea{
168+ anchors.fill: parent
169+ onClicked: {
170+ root.monthSelected(monthComponent.monthDate);
171 }
172 }
173 }

Subscribers

People subscribed via source and target branches

to status/vote changes: