-
Notifications
You must be signed in to change notification settings - Fork 8
Update tutorials #45
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: main
Are you sure you want to change the base?
Update tutorials #45
Conversation
|
@Sorixelle Seems GitHub's auto-merge main feature doesn't add the signed off by. Is that still an issue? |
|
@Sorixelle De-scoping the "Advanced Tutorials" section that contains only the PDC tutorial because it requires some files that I can't find anymore - broken link to the Pebble S3. It can be done in a follow-up PR. |
It is annoying but the checks don't like it, yeah. Generally, prefer rebases over merging to update a branch - it doesn't create an extra commit. Sorry it's taken me so long to get to this one - gotta find time to properly look through the changes. |
Gotcha, I'll update it soon. Thanks for your review! No problem 😄 |
54e0837 to
2443388
Compare
|
@Sorixelle Things seem happier now! |
Signed-off-by: C-D-Lewis <Bonsitm@gmail.com>
60bb24e to
b45f2cf
Compare
|
@Sorixelle Fixed lint error! Good to merge now I think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whew, sorry for taking so, so long to get around to this one. It's looking good, and I'm not fundamentally opposed to the changes. There's a few bits that need some cleanup though, I've left a handful of comments.
| services. | ||
|
|
||
| In this tutorial, we will interface with | ||
| [Open Weather Map](http://openweathermap.org/) – a common weather API used by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for future: rewrite to use OpenMeteo maybe? removes api key management requirment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a good thing to do, but for me at least it's out of scope of this housekeeping/fixes PR.
| If you have problems with your code, check it against the sample source code | ||
| provided using the button below. | ||
|
|
||
| [View Source Code >{center,bg-lightblue,fg-white}](https://github.com/pebble-examples/rocky-watchface-tutorial-part2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for removing this link? It still seems to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think now the entire source is in this page (easier to keep updated with the snippets leading to the full result, and easier for readers to compare) having some other entire copy to maintain is a burden we can do without. I felt similarly back in the day too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I was thinking of getting the tutorial watchface repos under this org anyway - maybe we can have both once that happens
|
|
||
| Why not let us know what you've created by tweeting | ||
| [@pebbledev](https://twitter.com/pebbledev), or join our epic developer | ||
| community on [Discord]({{ site.links.discord_invite }}). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can keep the Discord link here - maybe replace the Twitter link with the Fediverse page?
|
|
||
| Our first source file is already created for you by the `pebble` command | ||
| line tool and lives in the project's `src` directory. By default, this file | ||
| contains sample code which you can safely remove, since we will be starting from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unconvinced this is a necessary change
| #include <pebble.h> | ||
| ``` | ||
|
|
||
| After this first line, we must begin with the recommended app structure, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's not strictly "must", but I'm not sure "should" is best here either. Maybe just drop the word entirely and go "we begin with..."?
| as a starting template. For reference, that should look | ||
| [something like this](https://gist.github.com/pebble-gists/9b9d50b990d742a3ae34). | ||
| project or create a new one, using the code from the end of the last tutorial | ||
| as a starting point. Don't forget also to include changes to `package.json`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| as a starting point. Don't forget also to include changes to `package.json`. | |
| as a starting point. Don't forget to also include changes to `package.json`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like using the GitHub UI to resolve this is a trap for making the linter angry, so will do it on my end.
| [*App Resources*](/guides/app-resources/). All image files and fonts must | ||
| reside in subfolders of the `/resources` folder of your project. Below is an | ||
| example entry in the `media` array: | ||
| [*App Resources*](/guides/app-resources/). Below is an example entry in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| [*App Resources*](/guides/app-resources/). Below is an example entry in the | |
| [*App Resources*](/guides/app-resources/). Below is an example entry in the | |
| ```c | ||
| // Open AppMessage | ||
| const int inbox_size = 128; | ||
| const int inbox_size = 256; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a necessary code change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that this was a sensible value for someone who went from no settings to some medium length string or complex settings object and might be caught out later when it stops working. I'll update the comment for the value to explain.
| `package.json` such as the following: | ||
| Add this icon to your project by copying the above icon image to the | ||
| `/resources/images` project directory, and adding a new JSON object to the | ||
| `media` array in `package.json` such as the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `media` array in `package.json` such as the following: | |
| `media` array in `package.json`: |
| ## Conclusion | ||
|
|
||
| Now our watchface shows the watch's remaining battery level! It's discreet, | ||
| but very useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the summary for the part 4, should change this to match part 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 🦅 👁️
Signed-off-by: C-D-Lewis <bonsitm@gmail.com>
|
@Sorixelle Addressed the suggested changes or otherwise left comments in response. Please take a look again when able! |
Signed-off-by: C-D-Lewis <bonsitm@gmail.com>
|
@Sorixelle Ready to land! Thanks for looking at this one :) |
Goes some way to fixing #44 by testing creating projects from the snippets, and fixing dead links and small errors in code.
Also includes full source-code files as an expandable at the end instead of relying on a third-party Gist.
Advanced tutorials?Requires missing example files (PDC SVG Zip file)