Code review comment for lp:~aacid/unity8/action_preview_widget

27 + property var data: null

33 + onClicked: triggeredAction(data)

164 + root.triggered(root.widgetId, data.id, data);

306 + compare(spy.signalArguments[0][2], target.widgetData["actions"][buttonNumber]);

Those actions should not submit any data. Only actions triggered by data-generating widgets (rating-input, progress) will actually send any data.

Can we override the clicked() signal to send just the action id?

=====

62 + width: childrenRect.width
63 + height: childrenRect.height

I've a feeling this should be implicitWidth, but the buttons should be divided in two if the widget is wider.

Remember a single button should be right-aligned, too.

=====

81 + id: c

More name, please.

=====

92 + delegate: Button {

Why not PreviewActionButton?

=====

77 + width: c.maxWidth

82 + property real maxWidth: -1

98 + width: implicitWidth < parent.width ? parent.width : implicitWidth

100 + Component.onCompleted: {
101 + c.maxWidth = Math.max(c.maxWidth, implicitWidth);
102 + }

I don't think that's necessary, see above about widths.

=====

83 + anchors.top: moreButton.bottom
84 + anchors.topMargin: spacing

Compress into { } please.

=====

84 + anchors.topMargin: spacing

86 + spacing: height > 0 ? units.gu(1) : 0

Shouldn't the topMargin have that condition instead?

=====

97 + height: moreButton.expanded ? implicitHeight : 0

99 + visible: height > 0

Shouldn't that be on the Column instead?

=====

Animate the Column height please.

=====

142 + width: childrenRect.width

Should be parent-driven.

=====

144 + readonly property var actions: root.widgetData ? root.widgetData["actions"] : null

Move lower in the hierarchy, please, so it's not exposed.

=====

158 + item.model = actions.slice(1)

Smart! ++; please.

=====

174 +

--\n, please.

=====

185 + //! The id of the widget

ETOOMANYTHES, "The widget identifier"?

=====

301 + spy.clear();

→ cleanup()?

=====

303 + compare(spy.count, 1);

spy.wait();