davidedmundson added a comment.
Seems generally good.
INLINE COMMENTS
> config.qml:27
> +
> +Column {
> + id: root;
If you switch these to ColumnLayout / RowLayout (from QtQuick.Layouts, which
you're currently not using) you can get rid of a lot of the anchors and widths
in this code.
> config.qml:56
> + id: sizePositionRow;
> + spacing: units.largeSpacing / 2;
> +
it defeats the point of having spacing as semanticly defined macros, if people
then do maths with it to get any arbitrary value
> config.qml:106
> +
> + width: formAlignment - units.largeSpacing;
> + anchors.verticalCenter: bgColorButton.verticalCenter;
this won't acheive anything
you've set a width, but by default there's no eliding, so it'll just overflow
past here anyway.
> config.qml:264
> + id: bgColorDialog;
> + objectName: "bgColorDialog";
> +
why?
> main.qml:93
> + changeImage();
> + resetTimer();
> + }
where is resetTimer defined?
> main.qml:105
> + anchors.bottomMargin: Math.min((parent.height / 100) * 7.5,
> (parent.width / 100) * 7.5);
> + anchors.rightMargin: 5;
> + opacity: 0.25;
we generally try to avoid pixel sizes.
> main.qml:111
> + asynchronous: true;
> + mipmap: true;
> + }
why?
you're displaying this at a fixed size
REPOSITORY
rKDEPLASMAADDONS Plasma Addons
REVISION DETAIL
https://phabricator.kde.org/D1976
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: bgupta, #plasma
Cc: davidedmundson, plasma-devel, #plasma, jensreuterberg, sebas
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
