davidedmundson added inline comments.
INLINE COMMENTS
> FormLayout.qml:71
> + itemContainer.parent = lay;
> + //if section, wee need another placeholder
> + if (item.Kirigami.FormData.isSection) {
typo
> FormLayout.qml:85-86
> + property var item
> + implicitWidth: item.implicitWidth
> + Layout.preferredHeight: Math.max(item.Layout.preferredHeight,
> item.implicitHeight)
> +
why do something different for width and height?
Also I think we're better off doing
implicitheight: item.implicitHeight
Layout.preferredheight: item.Layout.preferredHeight
For layouts. Layout.preferred takes precedence over implicit if set, I think it
makes sense to just copy that.
> FormLayout.qml:113
> + id: buddyComponent
> + Kirigami.Heading {
> + id: labelItem
I'd like it if we can make this on the desktop style look the same as QGroupBox
looks in flat mode. Even if that means changing our QStyle.
Currently this means spanning both columns and then centre aligning.
> FormLayout.qml:117
> + text: item.Kirigami.FormData.label
> +
> + level: item.Kirigami.FormData.isSection ? 3 : 5
visible: item.visible might make sense.
it allows a dev to hide a UI component and label if it's not relevantl together
> FormLayout.qml:120
> +
> + Layout.preferredHeight: item.Kirigami.FormData.label.length > 0
> ? implicitHeight : Kirigami.Units.smallSpacing
> + Layout.alignment: root.wideMode ? (Qt.AlignRight |
> (item.Kirigami.FormData.buddyFor.height > height * 2 ? Qt.AlignTop :
> Qt.AlignVCenter))
why are we giving it a height if it's empty?
> FormLayout.qml:121
> + Layout.preferredHeight: item.Kirigami.FormData.label.length > 0
> ? implicitHeight : Kirigami.Units.smallSpacing
> + Layout.alignment: root.wideMode ? (Qt.AlignRight |
> (item.Kirigami.FormData.buddyFor.height > height * 2 ? Qt.AlignTop :
> Qt.AlignVCenter))
> + : (Qt.AlignLeft | Qt.AlignBottom)
In general with text, I think it's better to set the item to occupy the whole
space it /can/ take up, then use the Text.Alignment property to position the
text within it.
Otherwise if this wraps, your second line would start on the left not the right.
Related; what should happen if a user sets a really really really long label?
> FormLayout.qml:121-123
> + Layout.alignment: root.wideMode ? (Qt.AlignRight |
> (item.Kirigami.FormData.buddyFor.height > height * 2 ? Qt.AlignTop :
> Qt.AlignVCenter))
> + : (Qt.AlignLeft | Qt.AlignBottom)
> + Layout.topMargin: item.Kirigami.FormData.buddyFor.height >
> height * 2 ? Kirigami.Units.smallSpacing : 0
What's the rationale for this "if buddy.height > height *2" stuff?
REPOSITORY
R169 Kirigami
REVISION DETAIL
https://phabricator.kde.org/D8641
To: mart, #plasma, #kirigami, hein
Cc: colomar, ngraham, davidedmundson, plasma-devel, ZrenBot, progwolff,
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, hein
