ubuntu-touch-coreapps-reviewers team mailing list archive

[Nekhelesh Ramananthan <krnekhelesh@xxxxxxxxx>]

Review: Needs Fixing code-view & testing

That looks soooooooo much better! Awesome work! 

We're almost ready to merge this. Just one last final list of changes to be made,

1. The code uses a ListItem { Label { id: header } } to show the Header. I suggest we move that as well to using ListItemLayout which will automatically take care of eliding the text, positioning it and so on thereby reducing the amount of code. I did it and it works nicely. => http://paste.ubuntu.com/15258241/

2. Some nit-pick fixes w.r.t qml coding conventions,
a) Color codes should be in full caps..so "#EDEDED" instead of "#ededed". Makes it easier to spot them.

b) General structure of a QML Object is (notice the blank lines),

  Object {
    id
    objectName

    property declarations
    signal declarations
    JavaScript functions
    object properties
    child objects
    states
    transitions
  }

Can you ensure all objects (include the AgendaView Page obeys this)?

c) There are a few spelling mistakes in the comments written. For instance, // button to be showen when no calendar is selected (onClick will take user to list of all calendars) ... where it should be "shown" instead of "showen". 

3. It seems we're importing "import "dateExt.js" as DateExt"..but I don't see DateExt being used anywhere in the AgendaView. We can remove it safely.

4. Merge lp:ubuntu-calendar-app. You might get conflicts w.r.t calendar.qml since the background color was already changed to white in trunk by me..and you did it as well in this branch. If you're unsure how to resolve conflicts, let me know.

5. Let's get design opinion of the new agenda-view after the above items are fixed before merging this MP.

-- 
https://code.launchpad.net/~majster-pl/ubuntu-calendar-app/new-agenda-view/+merge/287448
Your team Ubuntu Calendar Developers is subscribed to branch lp:ubuntu-calendar-app.