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

Revision history for this message
MichaƂ Sawicz (saviq) wrote :

* the change to .bzrignore seems unrelated, please don't submit it
* I'd rename Bubble to Notification
* I don't see a reason to separate Bubble.qml into a separate dir - unless really difficult, there should only be a single Notification component that lays itself out according to the properties set
* "property bool snapDecision: actions != null" - this should depend on the type of the notification, not the presence of actions
* the whole bubble is 0.85 opaque, is that per-design? shouldn't just the background be transparent and the text fully opaque?
* please separate the components with newlines
* "visible: secondaryIconSource ? true : false" you can use just "source" here, secondaryIconSource is an alias anyway
* "left: parent.left; leftMargin: units.gu(8)" you should anchor to icon.right instead
* "fillMode: Image.PreserveAspectCrop" for secondaryIcon seems unnecessary, the icon should always be 2x2 GUs
* "leftMargin: secondaryIcon.visible ? units.gu(11) : units.gu(8)" anchor to icon or secondaryIcon instead
* "width: secondaryIcon.visible ? units.gu (25) : units.gu (28)" anchor to parent.right instead
* "font.family: "Ubuntu"" is default
* "opacity: 1.0" is default, and does _not_ mean that the text itself will be fully opaque, because the parent item is not opaque [1]
* "maximumLineCount: 1" is unnecessary when wrapping is disabled
* the above applies to bodyLabel, too, except that bodyLabel should anchor to summaryLabel's left and bottom
* you should elide the body, too, in case it overflows
* "width: units.gu (17.5)" is arbitrary, calculate the width based on parent.width
* "text: actions.get(1).action_label" you should almost never need Model.get() - use Views or Positioners with Repeaters [2] [3]
* the Loader is unnecessary, if you use a Repeater, it will not create anything if the model is empty
* use add/move/delete transitions

* in this case I'd rather see the layout to use Positioners [2] extensively for better readability and flexibility, like so:

Column {
    Row {
        UbuntuShape { id: icon }
        Image { id: secondaryIcon}
        Column {
            Label { id: summary }
            Label { id: body }
        }
    }
    Flow {
        Repeater { id: buttons }
    }
}

[1] http://qt-project.org/doc/qt-5.0/qtquick/qml-qtquick2-item.html#opacity-prop
[2] http://qt-project.org/doc/qt-5.0/qtquick/qtquick-qmltypereference.html#positioning
[3] http://qt-project.org/doc/qt-5.0/qtquick/qtquick-qmltypereference.html#model-view-types-and-data-storage-and-access

review: Needs Fixing

« Back to merge proposal