Ubuntu Calendar App

Merge lp://staging/~larochelle-brian/ubuntu-calendar-app/30MinIncrementsHackdays into lp://staging/ubuntu-calendar-app

  1. 30MinIncrementsHackdays
  2. Merge into trunk
Proposed by brian larochelle
Status: Merged
Approved by: Kunal Parmar
Approved revision: 225
Merged at revision: 223
Proposed branch: lp://staging/~larochelle-brian/ubuntu-calendar-app/30MinIncrementsHackdays
Merge into: lp://staging/ubuntu-calendar-app
Diff against target: 60 lines (+18/-4)
1 file modified
NewEvent.qml (+18/-4)
To merge this branch: bzr merge lp://staging/~larochelle-brian/ubuntu-calendar-app/30MinIncrementsHackdays
Reviewer Review Type Date Requested Status
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve
Kunal Parmar Approve
brian larochelle (community) Needs Resubmitting
Review via email: mp+213067@code.staging.launchpad.net

This proposal supersedes a proposal from 2014-03-27.

Commit message

Added logic to NewEvent.qml to choose a default of either 0 or 30 for minutes for the TimePicker. If minutes are past 30, an hour is added.

Description of the change

Added logic to NewEvent.qml to choose a default of either 0 or 30 for minutes for the TimePicker. If minutes are past 30, an hour is added.

To post a comment you must log in.
Revision history for this message
Kunal Parmar (pkunal-parmar) wrote : Posted in a previous version of this proposal

8 + startTime.defaultHour = (startDate.getMinutes() < 30) ? startDate.getHours() : startDate.getHours() + 1
19 + startTime.defaultMinute = (startDate.getMinutes() < 30) ? 30 : 0
20 + var popupObj = PopupUtils.open(timePicker,root,{"hour": startTime.defaultHour,"minute":startTime.defaultMinute});

Can we move above code to separate function ?

Revision history for this message
brian larochelle (larochelle-brian) wrote : Posted in a previous version of this proposal

sure.

I moved the logic to functions off the root of NewEvent {} and changed the ternary statements to if statements to improve readability.

Revision history for this message
Kunal Parmar (pkunal-parmar) wrote : Posted in a previous version of this proposal

Code looks great, Thanks.

I will test and approve it.

Revision history for this message
Kunal Parmar (pkunal-parmar) wrote : Posted in a previous version of this proposal

8 - var popupObj = PopupUtils.open(timePicker,root,{"hour": endDate.getHours(),"minute":endDate.getMinutes()});
39 + var popupObj = PopupUtils.open(timePicker,root,{"hour": root.hourForPickerFromDate(startDate),"minute":root.minuteForPickerFromDate(startDate)});

looks copy/paste error.

You are using startDate, instead of endDate

review: Needs Fixing
Revision history for this message
brian larochelle (larochelle-brian) wrote : Posted in a previous version of this proposal

Opps. I shouldn't have let that slip by. Thats what I get for trying to sneak this in at work... resubmitting.
thanks for the review.

Revision history for this message
brian larochelle (larochelle-brian) wrote :

Sorry again. I made another error. I'll take a little more time for this. Currently at work and was rushing to fix this.

review: Needs Fixing
Revision history for this message
Kunal Parmar (pkunal-parmar) wrote : Posted in a previous version of this proposal

:),

One more thing, that I am not sure of but.

Currently you are setting incremental value to time picker.

But do we need to set incremental time to start and end time label as well on construction ?

Revision history for this message
brian larochelle (larochelle-brian) wrote :

I was wondering that myself. I didn't see a clear pro or con, so I left it as is. I kinda like the label getting the real time at construction, but that preference isn't based on much. I'd be willing to change it, if that is decided.

lp://staging/~larochelle-brian/ubuntu-calendar-app/30MinIncrementsHackdays updated
225. By brian larochelle

Fix copy/paste error.
Increment the endDate by 30 instread of 10

Revision history for this message
brian larochelle (larochelle-brian) wrote :

Fixed copy/paste error.

also noticed endDate should have its minutes incremented by 30 instead of 10.
17 - endDate.setMinutes( endDate.getMinutes() + 10)
18 + endDate.setMinutes( endDate.getMinutes() + 30)

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

Thanks :), Looks great,
I will test once more and approve.

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

looks good to me

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: