Review Request: Weather Forecast QML port

[Luis Gabriel Lima]

> On Aug. 26, 2012, 9:36 p.m., David Edmundson wrote:
> > applets/weather/package/contents/ui/FiveDaysView.qml, line 70
> > <http://git.reviewboard.kde.org/r/106225/diff/1/?file=81340#file81340line70>
> >
> >     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.
> 
> Luis Gabriel Lima wrote:
>     There is no 'alpha' property in the Text element. Are you suggesting to use the 'opacity' property instead of a color with alpha channel?
> 
> David Edmundson wrote:
>     I meant opacity, yeah. AFAIK they're the same thing.

Nice. The difference is that the opacity property propagates the behavior for the children, but in this case as there is no element inside the Text I can use opacity.


> On Aug. 26, 2012, 9:36 p.m., David Edmundson wrote:
> > applets/weather/package/contents/ui/Notice.qml, line 27
> > <http://git.reviewboard.kde.org/r/106225/diff/1/?file=81341#file81341line27>
> >
> >     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
> 
> Luis Gabriel Lima wrote:
>     The C++ version uses the QApplication::font() instead of the plasma theme font. So after notice the difference when I was trying the PlasmaComponents.Label, I decided to keep the same way as the C++ version.
> 
> David Edmundson wrote:
>     That's sort of a good reason.
>     
>     Though I would argue that if the C++ one is wrong, there's no need to copy it.

I don't know if the C++ version is wrong, actually all the three plasmoid that I've ported was using QApplication::font().


> On Aug. 26, 2012, 9:36 p.m., David Edmundson wrote:
> > applets/weather/package/contents/ui/WeatherListView.qml, line 27
> > <http://git.reviewboard.kde.org/r/106225/diff/1/?file=81345#file81345line27>
> >
> >     18 hardcoded? Surely it depends on the size of the text?

I'll make some tests here to check.


- Luis Gabriel


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


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/20120829/b2134bc6/attachment.html>