D24305: [Cuttlefish] Compare an icon in different themes

[Nathaniel Graham]

ngraham added a comment.


  The QML is generally fine! Since you specifically requested it, I added nitpicky minor comments below. :) I don't consider them blockers, but feel free to take a look anyway!

INLINE COMMENTS

> Comparison.qml:1
> +import QtQuick 2.0
> +import QtQuick.Controls 2.5 as QQC2

2.4

> Comparison.qml:3
> +import QtQuick.Controls 2.5 as QQC2
> +import QtQuick.Layouts 1.0
> +

1.3

> Comparison.qml:65
> +                spacing: Kirigami.Units.largeSpacing
> +                 Component {
> +                    id: iconView

indentation

> Comparison.qml:69
> +                            width: comparisonSize
> +                            height: comparisonSize
> +                            source: modelData.iconPath

`height: width`

> Comparison.qml:70
> +                            height: comparisonSize
> +                            source: modelData.iconPath
> +                    }

Rather than creating a complicated additional item for the "no icon" case, I would just do `source: modelData.iconPath ? modelData.iconPath : "paint-none"`. If you don't like the `paint-none` icon, we can adjust it.

REPOSITORY
  R118 Plasma SDK

BRANCH
  comparison (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D24305

To: davidre, #plasma, #vdg, ngraham
Cc: ngraham, filipf, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20191001/cb91e8cb/attachment-0001.html>