broulik added inline comments.

INLINE COMMENTS

> EntryScreenshots.qml:133
>                      anchors.fill: thumbnail
> -                    verticalOffset: Kirigami.Units.largeSpacing
> -                    horizontalOffset: 0
> -                    radius: 12.0
> -                    samples: 25
> -                    color: Kirigami.Theme.disabledTextColor
> -                    cached: true
> +                    width: thumbnail.width;
> +                    height: thumbnail.height;

Not needed given you `anchors.fill`

> EntryScreenshots.qml:135
> +                    height: thumbnail.height;
> +                    Kirigami.Theme.inherit: false
> +                    Kirigami.Theme.colorSet: Kirigami.Theme.View

What's this for?

> EntryScreenshots.qml:138
> +                    shadow.xOffset: 0
> +                    shadow.yOffset: 0
> +                    shadow.size: Kirigami.Units.largeSpacing

You used `largeSpacing` as `verticalOffset` before

> EntryScreenshots.qml:139
> +                    shadow.yOffset: 0
> +                    shadow.size: Kirigami.Units.largeSpacing
> +                    shadow.color: Qt.rgba(0, 0, 0, 0.3)

You used `12` as `radius` before

> TileDelegate.qml:123
> +                        shadow.yOffset: 0
> +                        shadow.size: 10
> +                        shadow.color: Qt.rgba(0, 0, 0, 0.3)

`shadow.size` corresponds to the old `radius`, so you'd want to set 
`Kirigami.Units.largeSpacing` here

> TileDelegate.qml:124
> +                        shadow.size: 10
> +                        shadow.color: Qt.rgba(0, 0, 0, 0.3)
> +                    }

`#80` used in the old code is 128, i.e. `0.5` alpha, unless you want to make it 
look consistently with the shadows used elsewhere?

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #frameworks, #plasma, ahiemstra, broulik, mart
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

Reply via email to