davidedmundson added inline comments.
INLINE COMMENTS
> FormLayout.qml:8
> +
> +Control {
> + id: root
Why do we inherit from Control?
> FormLayout.qml:20
> + columnSpacing: Kirigami.Units.smallSpacing
> + anchors {
> + left: parent.left
this can be taller than the parent item.
> FormLayout.qml:27
> + default property list<Item> __items
> + on__ItemsChanged: {
> + for (var i = 0; i < __items.length; ++i) {
what about when an item gets deleted?
> FormLayout.qml:31
> +
> + if (item.parent && item.parent.parent == lay) {
> + continue;
so this is skipping items that are already in here?
> FormLayout.qml:47-48
> +
> + var buddy = buddyComponent.createObject(lay, {"formData":
> item.Kirigami.FormData})
> + buddy.parent = lay;
> +
merge
> FormLayout.qml:83
> + property var formData
> + width: 1
> + text: formData.label
If you're doing a hack round something, document what.
> FormLayout.qml:86
> +
> + font.pointSize: formData.isSection ?
> Kirigami.Theme.defaultFont.pointSize * 1.2 :
> Kirigami.Theme.defaultFont.pointSize
> + font.weight: formData.isSection ? Font.Light : Font.Normal
We have an entire KCM with fonts where you can customise everything.
Then we ignore it and hardcode a bunch of them. The end result is we have
complexity and don't even use it.
> formlayoutattached.h:33
> + Q_PROPERTY(bool isSection READ isSection WRITE setIsSection NOTIFY
> isSectionChanged)
> + Q_PROPERTY(QQuickItem *buddyFor READ buddyFor WRITE setBuddyFor NOTIFY
> buddyForChanged)
> +
Why would this ever not be the parent? I think having that writable is opening
a confusing mess you don't want.
> formlayoutattached.h:40
> +
> + void setLabel(const QString &text);
> + QString label() const;
That's a really clever idea; it makes it very flexible so a phone layout could
have the labels on top ++++
so I'm a bit surprised that the FormLayout.qml is an object, and not a
template; I think it's throwing away an opportunity.
REPOSITORY
R169 Kirigami
REVISION DETAIL
https://phabricator.kde.org/D8641
To: mart, #plasma, #kirigami, hein
Cc: davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart, hein
