Code review comment for lp:~macslow/unity/phablet-notification-renderer

W dniu 09.04.2013 17:07, Mirco Müller pisze:
>> 81 + width: (body == "") ? units.gu(2) : units.gu(6)
>> > 82 + height: (body == "") ? units.gu(2) : units.gu(6)
>> > we need to be able to collapse items that _do_ have body text, no? actually,
>> > it's the secondaryIcon that should stay on screen in collapsed state, even if
>> > there is a primary icon, no?
> No, this here is meant for the special "icon+summary" layout case Design wants. This is different from a collapsing snap-decision, which is only meant to be used on large screens.

Right, but since the icon is that small, do we not want to get rid of
the UbuntuShape for it? We could easily switch the source of the icons
instead of the dimensions of them? Is the lack of a body the only
determinant of whether it's a icon+summary layout? Does that mean body
is required for the generic case? It doesn't make sense for the icon to
be an avatar, for example, when it's just 2x2 GUs?

>> > 141 + visible: type == "Notifications.Type.SnapDecision"
>> > 145 + model: actions
>> > reference by object - notification.type, notification.actions
> That does not work. How can I make the object known? Do I need to change the way data is passed in the test?

Hum? The top level UbuntuShape has «id: notification» and type and
actions properties, can you not access those for some reason?

>> > 46 + width: parent.width
>> > 191 + anchors.fill: parent
>> > these need to go away (component dimensions should generally be handled in
>> > their parents, unless they depend on content dimensions):
> I could get rid of "anchors.fill: parent", but I have to keep "width: parent.width". That way the width of the ListView holding the notifications follows any embedding parent-item. Otherwise there's no way to tell the ListView what horizontal space it may occupy.

That's the «width: parent.width» in Notification.qml:29, it should be in
Notifications.qml:29 - it's in the delegate where you should define the
dimensions (again, unless they depend on their content). I think the
rule of thumb is: top-level component in a file should not have its
dimensions defined, unless they depend on the contents.

Case in point: it's Shell.qml that will decide on the dimensions of
Notifications {}, it's Notifications.qml that will decide on the
dimensions of Notification {} - it's delegate. And you should use
anchors there, too.

Cheers,
--
Michał Sawicz <email address hidden>
Canonical Services Ltd.