-
Notifications
You must be signed in to change notification settings - Fork 115
Features for ClockWidget and WeatherWidget #237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
|
Sorry, I just noticed there was a change to the config.h.template file.. I've uploaded a new copy to my branch. |
|
Your PR is greatly appreciated but you'll have a much better chance of getting these changes merged if you break this up into smaller PRs. Just a few suggestions. Try to keep one PR focused on only one feature or change. Also you have many many commits that don't have meaningful comments. It's best to have as few commits as possible and use comments that are meaning and relavant to tell code reviewers what, why, how you are changing this code. The other issue you'll have with too many changes in one PR is that your PR will quickly develop code conflict as it touches too many parts of the app. These code conflicts have to be resolved before you PR can be merged. Think about the PRs from the standpoint of the code reviewers. If the PRs are smaller and focused on only one change people are more likey to take the time to read the code and give you meaning feedback and help you get your changes into the code base. I'm interested in some of these changes myself particularly the OpenWeatherMap stuff |
|
Hi dreed47 |
I created a PR for setting the hostname #248 |
|
I'm starting to split these changes into smaller PR, first one is |
|
Let's keep this PR for reference, but merge the individual PRs. |
|
I just created a PR for the TZ api change in this package:
|
As posted on Discord, I've tried generating a pull request for the additional features I have developed