Review Request 128108: update weather applet ui

[Friedrich W. H. Kossebau]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128108/#review96287
-----------------------------------------------------------



Pretty good for your first QML patch :)

But there are a few issues that need to be fixed first and some which should be improved.
See the list of "Issues" below the screenshots at the "Reviews" page (which is the default view of this review request at https://git.reviewboard.kde.org/r/128108/)


applets/weather/package/contents/ui/TopPanel.qml (line 35)
<https://git.reviewboard.kde.org/r/128108/#comment65091>

    This change is not needed, and might be slightly more expensive, at least breaks the code pattern I saw in Plasma, so please revert it.
    
    Binding "width" to "height" of the same object is fine. And "height" is bound to "parent.height", so the code already should behave as you want it. If not, there would be a bug elsewhere, please report.



applets/weather/package/contents/ui/WeatherListView.qml (line 39)
<https://git.reviewboard.kde.org/r/128108/#comment65089>

    Instead of making the rectangle transparent, turn it into a normal undrawn Item, by removing the properties "radius" and "color" and changing the type to "Item" instead of "Rectangle", like:
    ```
            Item {
                id: rect
                height: root.rowHeight
                width: root.width
                Loader {
                [...]
    ```



applets/weather/package/contents/ui/main.qml (line 117)
<https://git.reviewboard.kde.org/r/128108/#comment65090>

    There is still "units.Spacing" set for the bottomMargin, which will yield 0. So if you want to set 0 here for both margins, best set that explicitely (not sure what the default is):
    ```
                    topMargin: 0
                    bottomMargin: 0
    ```
    
    Hm, having both top and bottom margin 0 now looks a little bit dense to me. But well, if you prefer that, okay.



applets/weather/package/contents/ui/main.qml (line 154)
<https://git.reviewboard.kde.org/r/128108/#comment65087>

    No need to change this, "color" is already bound to "theme.textColor" due to this being a PlasmaComponents.Label



applets/weather/package/contents/ui/main.qml (line 155)
<https://git.reviewboard.kde.org/r/128108/#comment65088>

    Please do not delete "textFormat: Text.StyledText", just add "opacity: 0.6".


- Friedrich W. H. Kossebau


On June 7, 2016, 11:46 a.m., Andreas Kainz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128108/
> -----------------------------------------------------------
> 
> (Updated June 7, 2016, 11:46 a.m.)
> 
> 
> Review request for Plasma and Friedrich W. H. Kossebau.
> 
> 
> Repository: kdeplasma-addons
> 
> 
> Description
> -------
> 
> the scaling work now better and the pading between the single lines can be smaler (If you make the applet higher you can also have the old line spacing)
> 
> it's my first qml change, so ...
> 
> on my todo, I'd like to change the powerd by weather.com text color so a secondary gray (bring it a bit more to the background), but I don't know how.
> 
> 
> Diffs
> -----
> 
>   applets/weather/package/contents/ui/TopPanel.qml 1821f16 
>   applets/weather/package/contents/ui/WeatherListView.qml c7b331e 
>   applets/weather/package/contents/ui/main.qml d490728 
> 
> Diff: https://git.reviewboard.kde.org/r/128108/diff/
> 
> 
> Testing
> -------
> 
> with breeze and breeze dark color scheme on two different pc's. the icons are breeze weather icons (master)
> 
> 
> File Attachments
> ----------------
> 
> screenshot
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/350136df-37c8-4fef-bddb-d4b609df2fb1__weather.png
> I hope I had fixed all open issues. I now have only one open issue I could change the linkColor but I can't change the color when there is no link like in noaa
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/06/06/3ef83b18-d8e3-4a73-a8da-a069c56a560a__weather.png
> 
> 
> Thanks,
> 
> Andreas Kainz
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160608/f494d1f6/attachment-0001.html>