----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
