Code review comment for lp:~yohanboniface/ubuntu-calendar-app/AgendaView

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

> Oh, you're right. But then what was you expecting me to do with this comment:
> https://code.launchpad.net/~yohanboniface/ubuntu-calendar-
> app/AgendaView/+merge/212164/comments/501716 ?

so, I wanted to you make root element of AgendaView a Page.

12 +Page {
13 + id: root
14 + objectName: "AgendaView"

Which you already did, I have no further comment there.

But, you are creating a Page and putting AgendaView ( which is also a page) inside this page, which is not necessary.
        + page: Page {
125 + anchors.fill: parent
126 + tools: commonToolBar
127 + AgendaView {

so, I think we can directly put AgendaView inside tab, instead of creating intermediate page. Like

+ Tab {
122 + objectName: "agendaTab"
123 + title: i18n.tr("Agenda")
124 + page: AgendaView {
                                  tools: commonToolBar
                                  // and rest of necessary properties
                             }

« Back to merge proposal