Code review comment for lp:~neokore/ubuntu-weather-app/CurrentWeatherComponent

Revision history for this message
David Planella (dpm) wrote :

Thanks Raúl for your branch!

While I won't be reviewing all of the branches, I thought I'd provide some feedback in the hope that it is useful for future merge proposals:

1) Remember to set the commit message, otherwise the branch cannot be autocommitted to trunk once accepted, and you'll get the warning messages from Ubuntu Phone Apps Jenkins Bot

2) On line 29 of your diff, you're committing code that's commented out. If it's not functional, it's better to remove it than to commit dead code

3) Around line 69 of your diff I see `text: currentTemp+"°"` which means you're hardcoding Centigrade degrees. I'd recommend against using hardcoded units, as they won't be useful for those using Fahrenheit in the US, for example. Is there no Qt library that handles localized units?

Other than that, looks good, thanks!

[1] Either use the "Set commit message" link near the top of the page, or the direct link at
 https://code.launchpad.net/~neokore/ubuntu-weather-app/CurrentWeatherComponent/+merge/151606/+edit-commit-message

review: Needs Fixing

« Back to merge proposal