Skip to content
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

add tutorial on how to add a new service #889

Merged
merged 19 commits into from Mar 30, 2017

Conversation

niccokunzmann
Copy link
Contributor

This was developed during #886.
I will see where I can link to this. Suggestions welcome.


This changes the index.html file automatically.

## (5) Create a Pull Request
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since contributors could be coming from any language or system, it's reasonable to guide them through the steps of using Node. However, let's assume they already know git and github. I appreciate that you want to make this a beginner tutorial, however including this text means we need to maintain it, and I don't see a lot of benefit in that. It's also effectively noise for most contributors, which detracts from the rest of the text. Could you replace it with a helpful link?

badgeData.colorB = '#008bb8'; // (4)
sendBadge(format, badgeData); // (5)
}));
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code would be a bit more readable if you lined up the numbers to the right. If you add js after the first set of backticks, GitHub will highlight it correctly.

2. Quit the running server with `Control+C`.
3. Start the server again.
`node server.js 8080`
4. Visit the badge at [http://[::1]:8080/test/asd/fgh.svg](http://[::1]:8080/test/asd/fgh.svg).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A screenshot would be helpful here, for clarifying what asd and fgh do. example1 might be a name than test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for me: todo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in a1da13e

----------------

Make sure, you can get the data somehow, best as JSON.
There might be an API for your service delivering the data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this mention down to 4.1, where you discuss querying an API?

7. Run the server
`node server.js 8080`
8. Visit the website to check the badges get loaded slowly:
[http://[::1]:8080/try.html](http://[::1]/try.html)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super-helpful!

`git clone git@github.com:YOURGITHUBUSERNAME/shields.git`
3. `cd shields`
4. Install npm
`sudo apt-get install npm nodejs-legacy curl`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Install imagemagick?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check if it was pre-installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for me: todo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. (it was installed on my Ubuntu)

<td><img src='/test/first/second.svg' alt=''/></td>
<td><code>https://img.shields.io/test/first/second.svg</code></td>
</tr>
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for including this.

There are various examples on how to answer with a customized badge:

- [docker automated builds](https://github.com/badges/shields/blob/bf373d11cd522835f198b50b4e1719027a0a2184/server.js#L5014)
- [the famous travis badge](https://github.com/badges/shields/blob/bf373d11cd522835f198b50b4e1719027a0a2184/server.js#L431)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would hurt to inline these examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recognized that you created a new tag for such contributions. I would rather refer to that tag than the two example: https://github.com/badges/shields/pulls?utf8=%E2%9C%93&q=is%3Apr%20label%3Anew-badge%20

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by inline?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for me: todo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulmelnikow Thanks for taking the time for feedback. This is the only comment I did not address. Could you tell me what inline means or what you expect? Maybe an example?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to give feedback, it's my pleasure. I should have explained better about inlining. I'm asking if you could take an example that uses an API, and paste the code block right into the tutorial, just like you did with the non-API example above. Pasting in the code makes it easier for the reader to study the patterns they should use.

You should read the following:

- [CONTRIBUTING.md](CONTRIBUTING.md)
- [INSTALL.md](INSTALL.md)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of INSTALL.md is irrelevant. Could you mention it below, for further reading?

@@ -0,0 +1,157 @@
Tutorial on how to add a badge for a service
============================================
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this to doc/TUTORIAL.md, and add a link to it in CONTRIBUTING.md?

@paulmelnikow
Copy link
Member

Thanks for this contribution! I left a handful of mostly minor comments. Think we could do a bit of consolidation of the developer docs, and this feels like a good start. 👍

doc/TUTORIAL.md Outdated
You can read

- previous successful pull-requests to see how other people implemented their badges.
You can as [merged pull-requests][mergedpr].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this sentence is missing a couple words? ^^

@paulmelnikow
Copy link
Member

Looking good! Thanks for all the edits.

@niccokunzmann
Copy link
Contributor Author

@paulmelnikow I pushed a description of the docker build as an example for an API query.
If you or an other person likes, you can review again.
I believe, I addressed all your comments from above.

@niccokunzmann
Copy link
Contributor Author

niccokunzmann commented Mar 30, 2017

I read through all of it. From my perspective I see nothing to improve.
I want to create an issue to fill the section about Authentication once this is merged.

@paulmelnikow paulmelnikow merged commit 21a3aef into badges:master Mar 30, 2017
@paulmelnikow
Copy link
Member

Thanks so much!

@niccokunzmann niccokunzmann deleted the add-starter-tutorial branch March 31, 2017 08:00
@niccokunzmann niccokunzmann restored the add-starter-tutorial branch March 31, 2017 08:01
@paulmelnikow paulmelnikow added the developer-experience Dev tooling, test framework, and CI label Apr 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants