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.

Sign up for GitHub

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

Jump to bottom

Docs: Update install instructions for Windows and Linux on the "Part 0" page #18262

Merged
merged 12 commits into from
Dec 12, 2019
Merged

Docs: Update install instructions for Windows and Linux on the "Part 0" page #18262

merged 12 commits into from
Dec 12, 2019

Conversation

1qk1
Copy link
Contributor

@1qk1 1qk1 commented Oct 7, 2019

Changes

  • Made homebrew a Mac-only requirement
  • Added detailed setup instructions for Windows, Linux, and MacOS without homebrew

Related Issues

#15529

@1qk1 1qk1 requested a review from a team as a code owner October 7, 2019 16:02
@1qk1 1qk1 changed the title Docs: Update install instructions for Windows and Linux Docs: Update install instructions for Windows and Linux on the "Part 0" page Oct 7, 2019
@wardpeet wardpeet added the type: documentation An issue or pull request for improving or updating Gatsby's documentation label Oct 9, 2019
@wardpeet wardpeet added the status: awaiting author response Additional information has been requested from the author label Oct 14, 2019
marcysutton
marcysutton suggested changes Oct 17, 2019
View reviewed changes
Copy link
Contributor

@marcysutton marcysutton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, and to @pieh and @wardpeet for the input! I've left one grammatical suggestion as well.

docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
@1qk1 1qk1 force-pushed the docs/update-setup-instructions branch from d331807 to 0057e98 Compare October 19, 2019 09:26
@1qk1
Copy link
Contributor Author

1qk1 commented Oct 25, 2019

I fixed your recommendations, can you check again? The force-push was because of a mistake, there was a commit for something that should be in another branch 😬

@laurieontech laurieontech mentioned this pull request Nov 20, 2019
Closed
This was referenced Dec 9, 2019
Merged
Closed
nickserv
nickserv previously approved these changes Dec 9, 2019
View reviewed changes
@laurieontech
Copy link
Contributor

@1qk1, let me know if you want me to move the changes over from the other doc. Happy to have you do it, but would like to get this merged in this week if we can!

@1qk1
Copy link
Contributor Author

1qk1 commented Dec 10, 2019

@1qk1, let me know if you want me to move the changes over from the other doc. Happy to have you do it, but would like to get this merged in this week if we can!

Sure! I'm on it.

@1qk1
Copy link
Contributor Author

1qk1 commented Dec 10, 2019

I will fix the errors in a while, my local environment wasn't working so I edited it online!

nickserv
nickserv suggested changes Dec 10, 2019
View reviewed changes
docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
laurieontech
laurieontech reviewed Dec 10, 2019
View reviewed changes
Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for porting all this over.

I made some changes to the flow so that it's hopefully easier to follow (more skipping around is necessary with all the different operating systems that won't apply to everyone).

Let me know what you think!

docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-zero/index.md Outdated Show resolved Hide resolved
docs/tutorial/part-zero/index.md Show resolved Hide resolved
@1qk1
Copy link
Contributor Author

1qk1 commented Dec 12, 2019

Thanks! I see a lot of them are about the unordered lists in the mac/windows/linux Node.js instructions, I agree, I'm not a big fan either, but didn't want it to take a lot of space. I will change them now.

@1qk1
Apply suggestions from code review
0c2c08c 
- Wording improvements
- Structure improvements

Co-Authored-By: LB <laurie@gatsbyjs.com>
@laurieontech
Copy link
Contributor

This is looking great! Adding a couple changes that got lost in the shuffle (there were lots of moving pieces). If we can update the picture and change those I think this will be good to go!

laurieontech
laurieontech reviewed Dec 12, 2019
View reviewed changes
docs/tutorial/part-zero/index.md Show resolved Hide resolved
docs/tutorial/part-zero/index.md Outdated

![Check node and npm versions in terminal](01-node-npm-versions.png)

Once you have followed the installation steps and you have checked everything is installed properly, you can continue to the next step:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Once you have followed the installation steps and you have checked everything is installed properly, you can continue to the next step:
Once you have followed the installation steps and you have checked everything is installed properly, you can continue to the next step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need to add this. "To confirm that this worked, you can run npm --version and node --version. The output should look similar to the screenshot below, showing version numbers in response to the commands."

Ooh I thought that was already there but I must've replaced it with the nvm commands and forgot about it.

@1qk1
Copy link
Contributor Author

1qk1 commented Dec 12, 2019 •
edited
Loading

I updated the image, my concern is the failed test, I think it's because this branch is way behind master and the dependencies have vulnerabilities?

@laurieontech
Copy link
Contributor

No worries about that. That failing test is actually a separate issue the team is solving.

@laurieontech
Copy link
Contributor

I think we're good to go here! Thanks so much for all your work @1qk1, this is going to help a whole bunch of people.

@laurieontech laurieontech merged commit e89d3f3 into gatsbyjs:master Dec 12, 2019
@1qk1 1qk1 deleted the docs/update-setup-instructions branch December 12, 2019 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pieh pieh pieh left review comments

@wardpeet wardpeet wardpeet left review comments

@jamo jamo jamo left review comments

@laurieontech laurieontech laurieontech left review comments

@nickserv nickserv nickserv requested changes

@marcysutton marcysutton marcysutton requested changes

Assignees
No one assigned
Labels
status: awaiting author response Additional information has been requested from the author type: documentation An issue or pull request for improving or updating Gatsby's documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@1qk1 @laurieontech @pieh @nickserv @marcysutton @wardpeet @jamo