Review Request: Weather Forecast QML port

[David Edmundson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106225/#review18054
-----------------------------------------------------------


Looks like a very good start. Comments below.


applets/weather/CMakeLists.txt
<http://git.reviewboard.kde.org/r/106225/#comment14295>

    Where do you use this?



applets/weather/package/contents/ui/FiveDaysView.qml
<http://git.reviewboard.kde.org/r/106225/#comment14297>

    What is this doing? Changing the text from inside "color: " isn't very intuitive to read. 
    
    You must be mixing logic and styling which isn't ever advised for maintainable code.
    
    Needs a comment at least, ideally rewriting.



applets/weather/package/contents/ui/FiveDaysView.qml
<http://git.reviewboard.kde.org/r/106225/#comment14296>

    you don't need this.. just use 
    
    color: theColour
    alpha: theAlpha
    
    merging them into a string, just for QML to split it again seems odd.



applets/weather/package/contents/ui/Notice.qml
<http://git.reviewboard.kde.org/r/106225/#comment14298>

    Unless there's a good reason, use PlasmaComponents.Label rather than Text. Otherwise font size won't be consistent.
    
    Saves you setting the theme colour yourself too :)
    
    Same for all instances of Text



applets/weather/package/contents/ui/WeatherListView.qml
<http://git.reviewboard.kde.org/r/106225/#comment14299>

    18 hardcoded? Surely it depends on the size of the text?



applets/weather/weatherapplet.h
<http://git.reviewboard.kde.org/r/106225/#comment14301>

    Having a slot with the same name as a signal is really confusing. You end up emitting dataUpdated from dataUpdated.
    



applets/weather/weatherapplet.h
<http://git.reviewboard.kde.org/r/106225/#comment14300>

    You don't need this.
    
    In your QML you can call plasmoid.openUrl()


- David Edmundson


On Aug. 26, 2012, 7:59 p.m., Luis Gabriel Lima wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106225/
> -----------------------------------------------------------
> 
> (Updated Aug. 26, 2012, 7:59 p.m.)
> 
> 
> Review request for Plasma and Marco Martin.
> 
> 
> Description
> -------
> 
> This patch is part of the work done in my GSoC 2012 project.
> 
> 
> Diffs
> -----
> 
>   applets/weather/CMakeLists.txt 4ae9c6f 
>   applets/weather/package/contents/ui/DetailsView.qml PRE-CREATION 
>   applets/weather/package/contents/ui/FiveDaysView.qml PRE-CREATION 
>   applets/weather/package/contents/ui/Notice.qml PRE-CREATION 
>   applets/weather/package/contents/ui/NoticesView.qml PRE-CREATION 
>   applets/weather/package/contents/ui/TopPanel.qml PRE-CREATION 
>   applets/weather/package/contents/ui/Utils.js PRE-CREATION 
>   applets/weather/package/contents/ui/WeatherListView.qml PRE-CREATION 
>   applets/weather/package/contents/ui/main.qml PRE-CREATION 
>   applets/weather/package/metadata.desktop PRE-CREATION 
>   applets/weather/weatherapplet.h a7e7f8d 
>   applets/weather/weatherapplet.cpp 9fd3e5f 
>   applets/weather/weatherdelegate.h 8a27f0d 
>   applets/weather/weatherdelegate.cpp 1cd987b 
>   applets/weather/weatherview.h f177501 
>   applets/weather/weatherview.cpp 5234cd3 
> 
> Diff: http://git.reviewboard.kde.org/r/106225/diff/
> 
> 
> Testing
> -------
> 
> - Tested inside panels and floating on desktop
> - Tested with various providers and locations
> 
> 
> Screenshots
> -----------
> 
> 
>   http://git.reviewboard.kde.org/r/106225/s/696/
> 
>   http://git.reviewboard.kde.org/r/106225/s/698/
> 
>   http://git.reviewboard.kde.org/r/106225/s/699/
> 
>   http://git.reviewboard.kde.org/r/106225/s/700/
> 
> 
> Thanks,
> 
> Luis Gabriel Lima
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20120826/825135da/attachment.html>