Create end-to-end and quick start guides for new contributors by mrjoro · Pull Request #7568 · ampproject/amphtml

mrjoro · 2017-02-15T06:12:47Z

Create guides to help new contributors who may not be familiar with Git/GitHub, Node, Gulp, etc.

the end-to-end guide provides detailed instructions and some background to help familiarize new contributors with the concepts needed to contribute to amphtml
the quick start guide is meant as a TL;DR version of the end-to-end guide for people who don't care as much about the concepts involved or to refresh the memories of people who may have had some experience in the past

cc @bpaduch @pbakaus @camelburrito

cramforce

Nice!

Are you planning to update DEVELOPING.md and CONTRIBUTING.md in a follow up?

cramforce · 2017-02-15T15:58:26Z

contributing/getting-started-quick.md

+
+* Add this line to your hosts file (`/etc/hosts` on Mac or Linux, `%SystemRoot%\System32\drivers\etc\hosts` on Windows):
+
+    ```127.0.0.1               ads.localhost iframe.localhost```


Renders fine, but maybe switch to multi line syntax to avoid confusing the highlighter here.

cramforce · 2017-02-15T16:00:17Z

contributing/getting-started-quick.md

+* Change the suffix for the examples from .html to .max.html to use your local JavaScript.
+
+# Test AMP
+* Run the tests: `gulp test`


Default to -w or mention it?

When I ran "gulp test --watch" I saw that it reran all tests and wondered why you'd want that. Then I saw the describe.only below and realized it works best when you do that.

Added bullet points for both (and updated the e2e guide).

cramforce · 2017-02-15T16:01:05Z

contributing/getting-started-quick.md

+
+# Test AMP
+* Run the tests: `gulp test`
+* Run a single test: `gulp test --files=<filename`


I personally prefer the describe.only method because it allows switching the tested files without restarting gulp test. It is slightly harder to explain, though.

I had to look this one up, and now it makes more sense why you'd run "gulp test --watch" since if you use describe.only it would only rerun those tests.

Added this as another bullet point.

cramforce · 2017-02-15T16:02:03Z

contributing/getting-started-quick.md

+# Pull the latest changes
+
+* `git checkout master`
+* `git pull --rebase upstream master`


--rebase is not needed here since there are by definition no changes.

@erwinmombay said the same, and then became convinced --rebase was needed after all. Erwin do you remember why?

I suppose since we're saying "never change master" then always at the tip of the history?

cramforce · 2017-02-15T16:03:15Z

contributing/getting-started-quick.md

+* pull the latest changes as described above
+* `git checkout <branch name>`
+* `git push origin <branch name>`
+* go to your fork on `https://github.com/<your username>/amphtml`


I use an easier process:

go to https://github.com/ampproject/amphtml

select the magic: "You have a new branch in your local fork, do you want to create a PR" button

Made that the default, but mentioned the other way in case the banner doesn't show up. (I'm not sure how long GitHub shows the banner.)

cramforce · 2017-02-15T16:04:17Z

contributing/getting-started-quick.md

+* choose your branch from the "Branch" dropdown and click "New pull request"
+* make sure you've signed the CLA (using the same email address as your git config indicates)
+* if your reviewer requests changes make them locally and then repeat the steps in this section to push the changes to your branch back up to GitHub again
+* once approved your changes are merged into the amphtml repository by a core committer (you don't do this merge)


Maybe add

"If you don't get a new review within 2 business days, always feel free to ping the pull request by adding a comment."

cramforce · 2017-02-15T16:04:37Z

contributing/getting-started-quick.md

+* if your reviewer requests changes make them locally and then repeat the steps in this section to push the changes to your branch back up to GitHub again
+* once approved your changes are merged into the amphtml repository by a core committer (you don't do this merge)
+
+# Delete your branch


Not sure if needed in this guide.

I figured I'd be complete. Made it clear this was optional and that it was for after the changes are merged.

mrjoro

I filed Issue #7569 to track updating the other docs to use these.

mrjoro · 2017-02-16T03:12:20Z

contributing/getting-started-quick.md

+# Pull the latest changes
+
+* `git checkout master`
+* `git pull --rebase upstream master`


@erwinmombay said the same, and then became convinced --rebase was needed after all. Erwin do you remember why?

I suppose since we're saying "never change master" then always at the tip of the history?

mrjoro · 2017-02-16T03:18:33Z

contributing/getting-started-quick.md

+* pull the latest changes as described above
+* `git checkout <branch name>`
+* `git push origin <branch name>`
+* go to your fork on `https://github.com/<your username>/amphtml`


Made that the default, but mentioned the other way in case the banner doesn't show up. (I'm not sure how long GitHub shows the banner.)

mrjoro · 2017-02-16T03:19:46Z

contributing/getting-started-quick.md

+* choose your branch from the "Branch" dropdown and click "New pull request"
+* make sure you've signed the CLA (using the same email address as your git config indicates)
+* if your reviewer requests changes make them locally and then repeat the steps in this section to push the changes to your branch back up to GitHub again
+* once approved your changes are merged into the amphtml repository by a core committer (you don't do this merge)


mrjoro · 2017-02-16T03:21:30Z

contributing/getting-started-quick.md

+* if your reviewer requests changes make them locally and then repeat the steps in this section to push the changes to your branch back up to GitHub again
+* once approved your changes are merged into the amphtml repository by a core committer (you don't do this merge)
+
+# Delete your branch


I figured I'd be complete. Made it clear this was optional and that it was for after the changes are merged.

mrjoro · 2017-02-16T03:23:54Z

contributing/getting-started-quick.md

+
+* Add this line to your hosts file (`/etc/hosts` on Mac or Linux, `%SystemRoot%\System32\drivers\etc\hosts` on Windows):
+
+    ```127.0.0.1               ads.localhost iframe.localhost```


mrjoro · 2017-02-16T03:36:59Z

contributing/getting-started-quick.md

+* Change the suffix for the examples from .html to .max.html to use your local JavaScript.
+
+# Test AMP
+* Run the tests: `gulp test`


When I ran "gulp test --watch" I saw that it reran all tests and wondered why you'd want that. Then I saw the describe.only below and realized it works best when you do that.

Added bullet points for both (and updated the e2e guide).

mrjoro · 2017-02-16T04:15:49Z

contributing/getting-started-quick.md

+
+# Test AMP
+* Run the tests: `gulp test`
+* Run a single test: `gulp test --files=<filename`


I had to look this one up, and now it makes more sense why you'd run "gulp test --watch" since if you use describe.only it would only rerun those tests.

Added this as another bullet point.

mrjoro

I updated the instructions for yarn after actually running it. :)

cramforce · 2017-02-17T03:52:13Z

contributing/getting-started-quick.md

+
+* Go to the master branch: `git checkout master`
+* Delete your local branch: `git branch -D <branch name>`
+* Delete the GitHub fork branch: `git push origin --delete <branch name>`


For follow up: Add information about AMP release cycle for when changes are expected to be available to all AMP pages!

…ject#7568)

adding initial versions of guides

f524c16

mrjoro requested a review from cramforce February 15, 2017 06:12

fixing relative links

7840047

mrjoro mentioned this pull request Feb 15, 2017

Update/reconcile contribution docs #7569

Closed

cramforce requested changes Feb 15, 2017

View reviewed changes

jridgewell added the Related to: Documentation label Feb 15, 2017

responding to review comments

db4c6ee

mrjoro commented Feb 16, 2017

View reviewed changes

mrjoro added 2 commits February 15, 2017 20:44

responding to review comments (missed one)

0cf3962

fixing instructions for yarn

ae12df3

mrjoro commented Feb 16, 2017

View reviewed changes

cramforce approved these changes Feb 17, 2017

View reviewed changes

cramforce merged commit 17e29ec into ampproject:master Feb 17, 2017

mrjoro deleted the add-guides branch February 18, 2017 03:13

mrjoro mentioned this pull request Feb 20, 2017

Weekly Status 2017-02-20 #7679

Closed

mrjoro added a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Create end-to-end and quick start guides for new contributors (amppro…

c9d98b1

…ject#7568)


		* Add this line to your hosts file (`/etc/hosts` on Mac or Linux, `%SystemRoot%\System32\drivers\etc\hosts` on Windows):

		```127.0.0.1 ads.localhost iframe.localhost```

Conversation

mrjoro commented Feb 15, 2017

Uh oh!

cramforce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrjoro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrjoro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants