broulik added a comment.
Skimmed through it until `ItemDelegate.qml`, few nitpicks for that below INLINE COMMENTS > BusyIndicator.qml:28 > + > + implicitWidth: contentItem.implicitWidth + leftPadding + rightPadding > + implicitHeight: contentItem.implicitHeight + topPadding + bottomPadding Apparently in Qt 5.8 the implicit sizes now take into account padding but since we want to support 5.7 (?) makes sense. Mentioned in the changelog: https://code.qt.io/cgit/qt/qtquickcontrols2.git/tree/dist/changes-5.8.0 > BusyIndicator.qml:41 > + > + anchors.centerIn: parent > + width: Math.min(control.width, control.height) You sure this doesn't break anything if you mess with anchors and sizes and don't have a wrapper `Item` around it? > BusyIndicator.qml:44 > + height: width > + smooth: !control.running || > (control.hasOwnProperty("smoothAnimation") && control.smoothAnimation) > + I think with QtQuick 2 `smooth` doesn't matter anymore, other than making it look worse. >From docs > In Qt Quick 2.0, this property has minimal impact on performance. > Button.qml:37 > + > + hoverEnabled: true //Qt.styleHints.useHoverEffects TODO: how to make > this work in 5.7? > + Docs say > You can also enable or disable hover effects for all Qt Quick Controls 2 > applications by setting the QT_QUICK_CONTROLS_HOVER_ENABLED environment > variable. > Button.qml:39 > + > + contentItem: Label { > + text: control.text What `Label` is this? I don't see any import/namespace that would provide this here > Button.qml:41 > + text: control.text > + font: control.font > + opacity: enabled || control.highlighted || control.checked ? 1 : 0.4 QQC2 `Label` supposedly supports `font` (and probably other) inheritance > CheckIndicator.qml:63 > + anchors.fill: parent > + state: control.activeFocus ? "focus" : (control.hovered ? "hover" : > "shadow") > + } Now that we have `activeFocusReason` we could potentially behave differently whether it was user-focused or tab-focused (not now but keeping that in mind for the future) > ComboBox.qml:96 > + > + popup: T.Popup { > + y: control.height in-window `Popup` makes me sad :( > DialogButtonBox.qml:41 > + > + contentItem: ListView { > + implicitWidth: contentWidth Do we really want a `ListView` here? Also, how does it work with e.g. `HelpRole` and things that would go on the left rather than the right? Dunno if it supports that though > Drawer.qml:35 > + > + topPadding: control.edge === Qt.BottomEdge ? 1 : 0 > + leftPadding: control.edge === Qt.RightEdge ? 1 : 0 `Math.round(units.devicePixelRatio)` instead of `1`? > Frame.qml:34 > + > + padding: 6 > + Hardcoded number REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D4508 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: mart, #plasma Cc: broulik, plasma-devel, #frameworks, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
