D8641: FormLayout

[David Edmundson]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20171107/0fa5f0df/attachment-0001.html>